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

[Mehdi AMINI via Phabricator via flang-commits]

mehdi_amini 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);
----------------
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.



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

https://reviews.llvm.org/D114104