This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Replace notifyMatchFailure with TODO hard failures
ClosedPublic

Authored by kiranchandramohan on Nov 22 2021, 8:16 AM.

Details

Summary

For unimplemented patterns we revert to using TODO hard failures instead of
notifyMatchFailure.

For fir.select_type revert to using mlir::emiterror.
For the fir.embox TODO on a type with len params we cannot add a test since the type cannot be converted to llvm.

Adding negative tests using not and checking for the error message.
TODO exits with an error in a build without assertion but aborts in a
build with assertions. Abort requires using not with the --crash
option. The two different usages of not is handled by using a custom
command %not_todo_cmd which is converted to not or not --crash
depending on the presence or absence of assertions. Using llvm-config
to check the presence of assertions.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2021, 8:16 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
kiranchandramohan requested review of this revision.Nov 22 2021, 8:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2021, 8:16 AM
clementval accepted this revision.Nov 22 2021, 8:22 AM
This revision is now accepted and ready to land.Nov 22 2021, 8:22 AM

This really improves the documentation, thanks! I've noticed a few Ops without tests - perhaps it's not possible to test these? Worth adding a note in the summary.

Also, could you explain the rationale behind the change in the commit message? We discussed this on Slack, but IMO it would make sense to briefly sum up here too. For our future selves.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
786

How about a test for this one?

800

A test file?

1563

Test?

clementval added inline comments.Nov 22 2021, 9:41 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
786

flang/test/Fir/Todo/dispatch.fir

800

flang/test/Fir/Todo/dispatch_table.fir

clementval added inline comments.Nov 22 2021, 9:42 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1563

This is one of the case where the type conversion will failed first before reaching this point.

awarzynski added inline comments.Nov 22 2021, 9:51 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
786

My bad, missed it, thanks!

800

My bad, missed it, thanks!

1563

Ta! Could this be documented in the summary?

mehdi_amini added inline comments.Nov 22 2021, 10:34 AM
flang/test/Fir/Todo/end.fir
1

Can you add 2>&1 | FileCheck %s and check that you actually capture the expected reason to fail?

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1563

will do.

flang/test/Fir/Todo/end.fir
1

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"() : () -> ()
mehdi_amini added inline comments.Nov 22 2021, 9:55 PM
flang/test/Fir/Todo/end.fir
1

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?

1

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.

flang/test/Fir/Todo/end.fir
1

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.

mehdi_amini added inline comments.Nov 22 2021, 11:19 PM
flang/test/Fir/Todo/end.fir
1

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?

mehdi_amini added inline comments.Nov 22 2021, 11:21 PM
flang/test/Fir/Todo/end.fir
1

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.

clementval added inline comments.Nov 23 2021, 12:58 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1169

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.

awarzynski added inline comments.Nov 23 2021, 1:39 AM
flang/test/Fir/Todo/end.fir
1

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.

kiranchandramohan edited the summary of this revision. (Show Details)

For fir.select_type revert to using mlir::emiterror.

Adding negative tests using not and checking for the error message.
TODO exits with an error in a build without assertion but aborts in a
build with assertions. Abort requires using not with the --crash
option. The two different usages of not is handled by using a custom
command %not_cmd which is converted to not or not --crash
depending on the presence or absence of assertions. Using llvm-config
to check the presence of assertions.

kiranchandramohan edited the summary of this revision. (Show Details)Nov 23 2021, 5:05 AM
kiranchandramohan marked an inline comment as done.Nov 23 2021, 5:08 AM
kiranchandramohan added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1169

Done. Thanks.

1563

Done.

flang/test/Fir/Todo/end.fir
1

I am going with @awarzynski's suggestion. Instead of %fir_opt_todo I am using %not_cmd.

mehdi_amini added inline comments.Nov 23 2021, 11:24 AM
flang/test/Fir/Todo/end.fir
1

Nice trick!

awarzynski accepted this revision.Nov 24 2021, 7:26 AM

I've left a few more nits - feel free to ignore. Thanks for implementing this! I think that you've also addressed all of Mehdi's comments, but please give it another day before merging (in case we missed sth).

flang/lib/Optimizer/CodeGen/CodeGen.cpp
545

FYI, I tried removing return failure(); and tested with

  • -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On, and
  • -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=Off.

No compiler errors/warnings in both cases. But it's probably safer to leave return failure(); here.

flang/test/lit.cfg.py
38 ↗(On Diff #389179)

This will only check for "asserts", right? Build type affects whether "asserts" are enabled, but that's just an implementation detail here. At this stage we just want to know whether the 'asserts' are "On", right?

78 ↗(On Diff #389179)

[nit] It would be helpful to have a comment explaining "why" we may need not with --crash (and, ultimately, %not_cmd).

80 ↗(On Diff #389179)

[nit] Wouldn't %not_or_crash be more descriptive than %not_cmd? Both not and not --crash are "commands", but:

//   not cmd
//     Will return true if cmd doesn't crash and returns false.
//   not --crash cmd
//     Will return true if cmd crashes (e.g. for testing crash reporting).

(from not.cpp). But perhaps that's more confusing?

kiranchandramohan marked 5 inline comments as done.

Rename not_cmd as not_todo_cmd. Add comments.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
545

OK. I will retain, incase some compiler warns.

flang/test/lit.cfg.py
38 ↗(On Diff #389179)

Thanks. Yes, this is a copy paste error, there is another flag (build-type) that we can query but as u point out, we do not use that here.

80 ↗(On Diff #389179)

Yes, I agree that not_cmd is not very descriptive. Since this is specific to the todo messages, I am replacing it with not_todo_cmd. This gives a hint that a developer should use not_todo_cmd when todo messages are involved. This along with the description should hopefully be enough.

kiranchandramohan edited the summary of this revision. (Show Details)Nov 30 2021, 4:43 AM
kiranchandramohan reopened this revision.Dec 2 2021, 1:40 AM
This revision is now accepted and ready to land.Dec 2 2021, 1:40 AM

Add the llvm install directory for finding tools in out of tree builds