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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan marked an inline comment as done.
kiranchandramohan added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1169
                   mlir::ConversionPatternRewriter &rewriter) const override {
-    return rewriter.notifyMatchFailure(
-        select, "fir.select_type codegen is not implemented yet");
+    TODO(select.getLoc(), "fir.select_type codegen");
+    return failure();
----------------
clementval wrote:
> This one is actually not a TODO. This high level op is not likely to have a codegen to LLVM IR. It will be converted before to lower level ops. 
Done. Thanks.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1563
+    if (isDerivedTypeWithLenParams(boxTy)) {
+      TODO(embox.getLoc(), "fir.embox codegen of derived with length parameters");
+      return failure();
----------------
kiranchandramohan wrote:
> awarzynski wrote:
> > clementval wrote:
> > > awarzynski wrote:
> > > > Test?
> > > This is one of the case where the type conversion will failed first before reaching this point. 
> > Ta! Could this be documented in the summary?
> will do.
Done.


================
Comment at: flang/test/Fir/Todo/end.fir:1
+// RUN: fir-opt --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" %s
+// XFAIL:*
----------------
awarzynski wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > kiranchandramohan wrote:
> > > > mehdi_amini wrote:
> > > > > mehdi_amini wrote:
> > > > > > 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?
> > > > > > The not prefix vs XFAIL is orthogonal to checking the message, my comment here is only about checking the cause of the failure.
> > > > > 
> > > > > Actually, no it isn't orthogonal. But the question remains: I'm not sure why the message isn't emitted in both cases.
> > > > The message is emitted in both cases. The failure in debug build happens because `not` fails on abort.
> > > > 
> > > > I see that there is a `--crash` option in `not` to handle this situation. I will give this a shot. This might require separate lines/tests for debug vs non-debug builds.
> > > Oh OK that's what I missed: I didn't know that `not` would not accept the crash situation by default!
> > > Maybe TODO can emit a "crashing error code" in non-debug build?
> > > Maybe TODO can emit a "crashing error code" in non-debug build?
> > 
> > Actually this may just not be possible, I'm not sure how we differentiate here in `not`.
> If the behavior is different for Debug and Release builds, then we could define %fir_opt_todo` in LIT that expands to different things depending on the build type.
I am going with @awarzynski's suggestion. Instead of %fir_opt_todo I am using %not_cmd.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114371