Convert fir.allocmem and fir.freemem operations to calls to malloc and free, respectively This patch is part of the upstreaming effort from the fir-dev branch. Co-authored-by: Eric Schweitz <eschweitz@nvidia.com> Co-authored-by: Jean Perier <jperier@nvidia.com> Address review comments - move CHECK blocks to after the mlir code in the test file - fix style with respect to anonymous namespaces: only include class definitions in the namespace and make functions static and outside the namespace - fix a few nits - remove TODO in favor of notifyMatchFailure - removed unnecessary CHECK line from convert-to-llvm.fir - rebase on main - add TODO back in - get successfull test of TODO in AllocMemOp converion of derived type with LEN params - clearer comments and reduced use of auto
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for working on this Alexis. Just a couple of comments before we can proceed.
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
80 | This function is just moved. Can you put it back at the same position so there is no change? | |
277 | Is this change needed? | |
299 | I think it's already in an anonymous namespace. | |
1026 | Can you remove the extra anonymous namespace? I think it's already in one. | |
1035 | To make auto people happy :-) | |
1042 | ||
1043 | ||
1085 | Same comment for the anonymous namespace | |
1093 | To make auto people happy :-) | |
flang/test/Fir/convert-to-llvm.fir | ||
172 | CHECK-LABEL? | |
172–176 | Can you move the check block after the MLIR code so it looks like other tests in the file? | |
189–198 | CHECK-LABEL and move the block after the MLIR code. |
Thanks so much for the review! I've been a little preoccupied of late, so this patch wasn't quite as polished as I would have like. Thanks for helping me get it in better shape!
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
80 | When I rebased on main I ended up with two of these same declarations in the file and apparently I chose the wrong one to remove. Thanks for catching that! | |
277 | I couldn't build the code without this change, so I would say yes? | |
299 | Thanks for the catch! | |
1026 | Whoops! Thanks for the catch (again)! | |
1035 | Thank you for this. Sometimes it can be hard for me to track down the types to replace auto with. | |
flang/test/Fir/convert-to-llvm.fir | ||
172–176 | Certainly. I was patterning after the llvm.unreachable example, but I have taken the liberty to fix that test so that it matches this style as well. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
299 | The coding standard indicates that anonymous namespace should be made as small as possible and only include class declarations. https://llvm.org/docs/CodingStandards.html#anonymous-namespaces |
A few nit comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1033 | nit: mlir::Type | |
1040 | Nit: We have been using notifyMatch failure instead of TODOs in upstream. | |
1092 | Not for this patch: If we can write a wrapper function that creates the bitcast if the types are different but does not if they are the same, it might save a bitcast. LLVM will anyway remove, so not very important. | |
flang/test/Fir/convert-to-llvm.fir | ||
213–216 | Nit: Is there a change here? If not it might be good to keep it as is. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | That's rather unfortunate since the point of the TODO macro was to make them easy to grep for. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | I have been kind of serially requesting this change after the discussion in https://reviews.llvm.org/D113014#inline-1078096 which pointed out that conversion patterns should not directly emit errors. On the other hand, there are also concerns (@awarzynski @clementval) that we are not able to capture the diagnostic generated by the notifyMatchFailure in invalid verify tests. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | TODO and notifyMatch are very different: the first one is intentionally crashing the compiler at this point for something unimplemented. The notifyMatch is a graceful "failed to apply this pattern". If you look at the revision link: I didn't comment on a TODO but on something that was emitting a diagnostic and gracefully failing to apply the pattern. This is problematic from an API contract point of view since failing to apply a pattern is intended to be a recoverable thing, so it isn't a good point for general diagnostics in principle. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | OK. Thanks for the clarification. I will create a patch to re-insert the TODOs. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | The problem with TODO is that we cannot add tests for these situations since it will crash with a debug build. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | I'm confused. Does notifyMatch actually work for testing? Why can't a hard error, like TODO, be caught and examined in a test? I don't think there is really anything graceful to do when running into unimplemented holes in the code. In this case, if a rewrite is not implemented, there's not any real fall back or re-do option that's very satisfactory. For first impressions, we do not want to emit broken object code (with missing functionality) and have users come away with the impression something "worked" at compile time but then be disappointed that the executable failed. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 |
TODO is always crashing, not only in debug build AFAIK. I believe the only difference is that in a debug build you get a backtrace. You can still test for it if you'd like though: // RUN: not fir-opt --pass-that-crashes 2>&1 | FileCheck %s // CHECK: not yet implemented fir.allocmem of derived type with length parameters
The message issued by notifyMatchFailure is hook into a debug option that is compiled out in release builds (like llvm::dbg() or LLVM_DEBUG(x) macro). Also the framework can recover from a match failure (there may be another lowering that will catch an op): a "pattern application failure" isn't an error in itself. That said there is https://reviews.llvm.org/D110896 that is trying to offer an option to collect the notifyMatchFailure messages out-of-band, but we don't have agreement to enable this in non-debug build right now. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 |
The difference is in how we exit.
This works with a release build, but not in a debug build due to the issue mentioned above.
Currently, we are not able to recover the exact reason for a match failure. The existing tests just checks that legalization for an FIR operation fails. For e.g. for the zero_bits operation we cannot recover the info that it failed due to aggregate types. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
299 | Thank you for the review. I think I have implemented your suggestions correctly, but I would appreciate it if you could verify. There were a number of places I had to change that were outside of the particular part of the file I had been working on and I want to make sure I captured everything appropriately. | |
1040 | Ok, so should I leave the TODO in for this particular patch? | |
flang/test/Fir/convert-to-llvm.fir | ||
213–216 | The change is moving the CHECK lines to after the the test to match the style of the rest of the file. |
Address review comments
- move CHECK blocks to after the mlir code in the test file
- fix style with respect to anonymous namespaces: only include class definitions in the namespace and make functions static and outside the namespace
- fix a few nits
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | I asked in the other revision (sorry I missed this one),
I don't quite get the problem: I'd expect mlir::emitError to emit the message, and then the abort to lead to a long backtrace. But in both case you can CHECK to match the error message. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | I'm glad that we are having this discussion on TODO vs notifyMatchFailure - IMO we should always test for the generated diagnostics (such tests are great reproducers for the corresponding code paths, among other things)!
@mehdi I suggest that in this review we focus on fir.allocmem and fir.freemem :) Shall we move this discussion to https://reviews.llvm.org/D114371?
@AlexisPerry IMO this patch should be consistent with the current implementation in this file. In D114371 we can decide whether this should be a TODO or notifyMatchFailure (and refactor accordingly). BTW, would you be able to write a test to exercise this error? |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
1040 | Thanks @awarzynski . I have made the change. Hopefully everything looks good now. Regarding the test, I haven't used notifyMatchFailure before and would really appreciate it if you could point me to a good example to pattern after. |
Thanks for working on this @AlexisPerry!
would really appreciate it if you could point me to a good example to pattern after.
Sure! Looking at the code from your patch I am guessing that you'd need to test it with a record type (i.e. fir.type) with at least one LEN parameter:
if (auto recTy = fir::unwrapSequenceType(heap.getType()).dyn_cast<fir::RecordType>()) if (recTy.getNumLenParams() != 0) return rewriter.notifyMatchFailure( loc, "fir.allocmem of derived type with length parameters");
Here's an example of such type. You can probably simplify it a bit. Once you have an example that hits notifyMatchFailure in AllocMemOpConversion, you can use it to create a test in convert-to-llvm-invalid.fir.
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
894 | I feel that we should use indexType() here instead (otherwise this duplicates that method). This would probably require turning this into a member method though. Alternatively, the result of indexType() could be one of the parameters. | |
912 | ||
921 | Could you add a test that triggers this code-path? I'd try with multi-dim arrays. | |
963 | This is a nit. If you agree with this suggestion, could you also update other function comments? | |
971 | ||
flang/test/Fir/convert-to-llvm.fir | ||
197 | What's ONE for? If it's not used then perhaps it should not be generated? |
rebase on main - add TODO back in
get successfull test of TODO in AllocMemOp converion of derived type with LEN params
Address review comments - add a test that triggers the TODO in AllocMemOp Conversion
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
914 | @schweitz @jeanPerier Is this an acceptable change from heap.getType()? I needed this in order to be able to trigger the error. | |
flang/test/Fir/convert-to-llvm.fir | ||
197 | Oops! That was leftover from a previous version of the test. It's not needed now, so I'll remove it. Thanks for the catch! |
I believe I have corrected all the build errors and addressed all the comments. There should now be appropriate working testing as well. Is this patch ready to go? Thanks!
Thanks for working on this and addressing the comments!
I've left one comment inline. I'm also rather unsure about the "namespace" changes, which are not required for the conversions implemented here. These are refactoring changes, tangential to the functionality introduced here and IMO deserve a dedicated patch. Perhaps this was requested in an earlier conversation here? @clementval , @kiranchandramohan , @mehdi_amini , @schweitz WDYT?
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
277 | @AlexisPerry This is an unrelated change and IMO it shouldn't be included here. Perhaps earlier you were getting build errors due to some other issues? Could you try again? If you are getting build errors, could you share the log with us? |
Thanks @awarzynski ! Yes, the namespace changes (and use of "static") were requested in a previous comment on this review :
The coding standard indicates that anonymous namespace should be made as small as possible and only include class declarations.
Functions should be made static and outside the namespace instead.https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
Since this is clearly a case of making something match the LLVM coding standards (and the original code on fir-dev that this patch was taken from matched this particular part of the standards to begin with), I see no need to split these simple changes out into a separate patch.
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
277 | I had been getting build errors on my local machine specifically calling out this line and complaining that there was no variable name provided. However, now these build errors have magically disappeared, so I am updating the diff accordingly. |
LGTM.
Thanks @AlexisPerry for this patch. I have some info and a request for a test.
Apologies for hijacking the patch with all the discussion about todo vs notify.
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
41 | I just submitted a patch with this function. So you will have to remove on rebase. | |
906 | This is already in. | |
936 | I did not see an llvm.mul op in a test. Did we miss a case? Can we try the following? subroutine ss2(M,N) integer :: M, N real :: aa(M,N) end func @_QPss2(%arg0: !fir.ref<i32>, %arg1: !fir.ref<i32>) { %0 = fir.load %arg0 : !fir.ref<i32> %1 = fir.convert %0 : (i32) -> index %2 = fir.load %arg1 : !fir.ref<i32> %3 = fir.convert %2 : (i32) -> index %4 = fir.alloca !fir.array<?x?xf32>, %1, %3 {bindc_name = "aa", uniq_name = "_QFss2Eaa"} return } or subroutine sb(y,n) integer :: y, n character(len=y) :: s(n) end subroutine func @_QPsb(%arg0: !fir.ref<i32>, %arg1: !fir.ref<i32>) { %0 = fir.load %arg0 : !fir.ref<i32> %3 = fir.load %arg1 : !fir.ref<i32> %4 = fir.convert %3 : (i32) -> index %6 = fir.alloca !fir.array<?x!fir.char<1,?>>(%0 : i32), %4 {bindc_name = "s", uniq_name = "_QFsbEs"} return } |
@AlexisPerry I will add the additional tests.
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
41 | @mehdi_amini Is "static inline" OK here, or is there any guidance that requires "static"? |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
41 | There are lots of occurrences in the LLVM codebase so I would assumed it's fine. |