[PATCH] D96454: [flang][fir] Update the code generation test tool

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/include/flang/Optimizer/Support/FatalError.h:33
+  llvm::report_fatal_error(message);
+}
+
----------------
This function is just basically "renaming" `llvm::report_fatal_error`? It isn't clear to me why we wouldn't just use `llvm::report_fatal_error`?


================
Comment at: flang/lib/Optimizer/Support/FIRContext.cpp:23
+void fir::setTargetTriple(mlir::ModuleOp mod, llvm::Triple &triple) {
+  mod->setAttr(tripleName, fir::OpaqueAttr::get(mod.getContext(), &triple));
+}
----------------
mehdi_amini wrote:
> Opaque attribute are supposed to be non-semantically impactful I believe and be able to be dropped, Triple does not seems to fit here.
> 
> I actually don't know about uniquer and kindmap, but this is making me nervous as well.
Seems like your patch update messed up Phabricator comments handling, I can't see your answer here, but I think you wrote:

> Your objections are noted. We'd like to table this for the moment so as to continue upstreaming.

The point of reviewing this kind of thing from your out-of-tree prototype to upstream is to address this. We can't just table this.

(Also I don't see anything testing the Opaque attributes right now.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96454