[PATCH] D114371: [Flang] Replace notifyMatchFailure with TODO hard failures

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/test/Fir/Todo/end.fir:1
+// RUN: fir-opt --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" %s
+// XFAIL:*
----------------
kiranchandramohan wrote:
> mehdi_amini wrote:
> > Can you add  `2>&1 | FileCheck %s` and check that you actually capture the expected reason to fail?
> I am assuming you mean to prefix with not, redirect error to stdout and check with FileCheck after removing XFAIL, like given below. 
> 
> As I mentioned in the other thread, this does not work in a debug build. In a debug build, fir::emitFatalError is called which calls mlir::emitError and then aborts with llvm_report_fatal_error. I think the abort makes it difficult to test. 
> https://github.com/llvm/llvm-project/blob/0f652d8f527f3743771c8ad70f47d1019cb7ca1a/flang/include/flang/Lower/Todo.h#L65
> https://github.com/llvm/llvm-project/blob/0f652d8f527f3743771c8ad70f47d1019cb7ca1a/flang/include/flang/Optimizer/Support/FatalError.h#L23
> 
> 
> 
> 
> ```
> // RUN: not fir-opt --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" %s 2>&1 | FileCheck %s
>   
> // Test `fir.end` conversion to llvm.
> // Not implemented yet.
> 
> // CHECK: not yet implemented fir.end codegen
> 
> "fir.end"() : () -> ()
> ```
> I am assuming you mean to prefix with not, redirect error to stdout and check with FileCheck after removing XFAIL, like given below.

The `not` prefix vs XFAIL is orthogonal to checking the message, my comment here is only about checking the cause of the failure.


>  In a debug build, fir::emitFatalError is called which calls mlir::emitError and then aborts with llvm_report_fatal_error. I think the abort makes it difficult to test. 

I'm puzzled: wouldn't the message show up anyway? 
Why wouldn't the `// CHECK: not yet implemented fir.end codegen` work in both mode?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114371/new/

https://reviews.llvm.org/D114371