This is an archive of the discontinued LLVM Phabricator instance.

[FIR] Convert fir.allocmem and fir.freemem operations to calls to malloc and free, respectively
ClosedPublic

Authored by AlexisPerry on Nov 17 2021, 10:07 AM.

Details

Summary
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

Diff Detail

Event Timeline

AlexisPerry created this revision.Nov 17 2021, 10:07 AM
AlexisPerry requested review of this revision.Nov 17 2021, 10:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 10:07 AM
AlexisPerry retitled this revision from Convert fir.allocmem and fir.freemem operations to calls to malloc and free, respectively to [FIR] Convert fir.allocmem and fir.freemem operations to calls to malloc and free, respectively.Nov 17 2021, 10:08 AM

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.

AlexisPerry marked 10 inline comments as done.

Address review comments.

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.

AlexisPerry marked an inline comment as done.Nov 17 2021, 12:30 PM
mehdi_amini added inline comments.Nov 17 2021, 4:39 PM
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.
Functions should be made static and outside the namespace instead.

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.

schweitz added inline comments.Nov 18 2021, 3:00 PM
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.

mehdi_amini added inline comments.Nov 18 2021, 3:47 PM
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.

schweitz added inline comments.Nov 18 2021, 6:03 PM
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.

mehdi_amini added inline comments.Nov 18 2021, 7:22 PM
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.

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

Does notifyMatch actually work for testing

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

Why can't a hard error, like TODO, be caught and examined in a test?

The problem with TODO is that we cannot add tests for these situations since it will crash with a debug build.

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.

The difference is in how we exit.
-> In a release build, mlir::emitError is called and we exit with status 1. https://github.com/llvm/llvm-project/blob/0f652d8f527f3743771c8ad70f47d1019cb7ca1a/flang/include/flang/Lower/Todo.h#L43
-> 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. Is it OK to replace the abort with exit(1) just like in a release build?
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

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

This works with a release build, but not in a debug build due to the issue mentioned above.

Does notifyMatch actually work for testing

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.

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.
https://github.com/llvm/llvm-project/blob/0f652d8f527f3743771c8ad70f47d1019cb7ca1a/flang/test/Fir/convert-to-llvm-invalid.fir#L9
https://github.com/llvm/llvm-project/blob/0f652d8f527f3743771c8ad70f47d1019cb7ca1a/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L1252

flang/lib/Optimizer/CodeGen/CodeGen.cpp
1040

I have created D114371 to revert to using TODOs. Using XFAIL tests for these instead of verify tests. Please have a look.

AlexisPerry marked an inline comment as done.Nov 22 2021, 12:42 PM
AlexisPerry added inline comments.
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

Rebase on main

mehdi_amini added inline comments.Nov 22 2021, 1:03 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
41
298–299
299
299

Just pointed two places, otherwise LG

AlexisPerry marked 6 inline comments as done.Nov 22 2021, 1:17 PM

Fix a couple namespacing errors

flang/lib/Optimizer/CodeGen/CodeGen.cpp
299

Thank you!

AlexisPerry marked an inline comment as done.

Fix a couple namespacing errors

mehdi_amini added inline comments.Nov 22 2021, 9:58 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
1040

I asked in the other revision (sorry I missed this one),

-> 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. Is it OK to replace the abort with exit(1) just like in a release build?

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.
(we should likely keep the discussion on either this revision or the other)

awarzynski added inline comments.Nov 23 2021, 2:04 AM
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)!

(we should likely keep the discussion on either this revision or the other)

@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?

Ok, so should I leave the TODO in for this particular patch?

@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?

Remove TODO in favor of notifyMatchFailure

AlexisPerry added inline comments.Nov 23 2021, 11:18 AM
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

AlexisPerry marked an inline comment as done.

rebase on main - add TODO back in
get successfull test of TODO in AllocMemOp converion of derived type with LEN params

AlexisPerry marked an inline comment as done.Dec 3 2021, 11:36 AM

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!

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

Address review comments - clearer comments and reduced use of auto

AlexisPerry marked 15 inline comments as done.

rebase on upstream main

Fixed build error - removed duplicate function definition

move definition of computeDerivedTypeSize to fix build error

Rebase on main

Fix whitespace errors (hopefully)

fix build error

clang-format

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?

AlexisPerry marked an inline comment as done.

address latest review comments

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.

awarzynski accepted this revision.Dec 7 2021, 8:03 AM

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.

SGTM, thanks for working on this!

This revision is now accepted and ready to land.Dec 7 2021, 8:03 AM

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
40–45

@mehdi_amini Is "static inline" OK here, or is there any guidance that requires "static"?

clementval added inline comments.Dec 10 2021, 6:21 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
40–45

There are lots of occurrences in the LLVM codebase so I would assumed it's fine.