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

Mon Nov 22 12:42:23 PST 2021

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

================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:245

+namespace {
+/// Helper function for generating the LLVM IR that computes the size
----------------
mehdi_amini wrote:
> AlexisPerry wrote:
> > clementval wrote:
> > > I think it's already in an anonymous namespace.
> > Thanks for the catch!
> 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
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.

================
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);
----------------
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?

================
Comment at: flang/test/Fir/convert-to-llvm.fir:214-217
+// CHECK:  llvm.func @test_unreachable() {
+// CHECK-NEXT:    llvm.unreachable
+// CHECK-NEXT:  }
+
----------------
kiranchandramohan wrote:
> Nit: Is there a change here? If not it might be good to keep it as is.
The change is moving the CHECK lines to after the the test to match the style of the rest of the file.

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

https://reviews.llvm.org/D114104