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

Thu Nov 18 17:15:29 PST 2021

kiranchandramohan 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:
> 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.

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

https://reviews.llvm.org/D114104