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

[Eric Schweitz via Phabricator via llvm-commits]

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


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

https://reviews.llvm.org/D114104