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

Tue Nov 23 11:18:51 PST 2021

AlexisPerry added inline comments.

================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:812
+      if (recTy.getNumLenParams() != 0)
+        TODO(loc, "fir.allocmem of derived type with length parameters");
+    mlir::Value size = genTypeSizeInBytes(loc, ity, rewriter, ty);
----------------
awarzynski wrote:
> mehdi_amini wrote:
> > AlexisPerry wrote:
> > > kiranchandramohan wrote:
> > > > kiranchandramohan wrote:
> > > > > mehdi_amini wrote:
> > > > > > schweitz wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > kiranchandramohan wrote:
> > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > schweitz wrote:
> > > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > > Nit: We have been using notifyMatch failure instead of TODOs in upstream.
> > > > > > > > > > > > That's rather unfortunate since the point of the TODO macro was to make them easy to grep for.
> > > > > > > > > > > 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.
> > > > > > > > > > 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.
> > > > > > > > > OK. Thanks for the clarification.
> > > > > > > > > 
> > > > > > > > > I will create a patch to re-insert the TODOs.
> > > > > > > > The problem with TODO is that we cannot add tests for these situations since it will crash with a debug build.
> > > > > > > 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.
> > > > > > > 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.
> > > > > > 
> > > > > > 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
> > > > I have created D114371 to revert to using TODOs. Using XFAIL tests for these instead of verify tests. Please have a look.
> > > Ok, so should I leave the TODO in for this particular patch?
> > 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)
> 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?
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.

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

https://reviews.llvm.org/D114104