[PATCH] D95399: [flang][fir] Upstream FIR dialect changes.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRDialect.h:49
+  explicit FIRCodeGenDialect(mlir::MLIRContext *ctx);
+  virtual ~FIRCodeGenDialect();
+
----------------
schweitz wrote:
> mehdi_amini wrote:
> > schweitz wrote:
> > > mehdi_amini wrote:
> > > > I don't find a definition for these methods right now?
> > > See https://reviews.llvm.org/D95401
> > OK, but this patch can't be built / tested at the moment in this state I think.
> Per the feedback received on the flang community calls regarding the upstreaming plan, these diffs are meant to follow the proposed plan. The FIR "chunk" being broken down into a small series of linked reviews. In aggregate, the diffs compose a buildable and testable merge.
> 
Ah that wasn't clear to me what you were trying to do here.

I'm still not on-board with not landing these incrementally though. Many pieces in this patch can easily be split, tested, and landed independently of a large blob.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRDialect.h:68
   // clang-format on
+  registry.loadAll(&ctx);
 }
----------------
schweitz wrote:
> mehdi_amini wrote:
> > schweitz wrote:
> > > mehdi_amini wrote:
> > > > Why is this needed? In general this indicates a misconfiguration of the pipeline and shouldn't be used.
> > > > 
> > > > (also this method is not used in this patch)
> > > The test tools call this to register the dialects they'll use. If there is some preferred way of forcing that to happen, can you provide a pointer? Thanks.
> > The dialect registered don't need to be loaded in the context, just inserting them should be enough for parsing any input containing these dialects.
> > 
> > If there are passes converting a dialect into another one, the destination dialect can be declared through https://mlir.llvm.org/docs/PassManagement/#dependent-dialects
> Thanks for the reference. It's not part of these diffs, but it is probably worth mentioning that the flang front-end is not structured as an MLIR pass.
Ah right of course, but the loading (and so this function) should be located with the frontend components that will emit these dialects, not with the dialect itself.

Is the frontend really directly emitting all of the dialects above?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95399