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


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

https://reviews.llvm.org/D114104