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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

(please ping when you get test coverage)



================
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:
> > > > 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?
> > 
> This was a convenient place to put this list, which is shared in common between the bridge test tools.
> 
> We do use all of the dialects in the list.
> 
The list is fine, I'm mostly looking at the loading aspect: you could decouple this by having this function take a registry, and the caller be in charge of pre-loading or not (the frontend should pre-load, the opt testing should should not).
(Basically, keep the existing method as is, loading is a caller problem)


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRType.h:451
+  }
+};
+
----------------
Can this all be defined in TableGen?


================
Comment at: flang/test/Fir/fir-ops.fir:4
 // RUN: tco -emit-fir %s | tco -emit-fir | FileCheck %s
-// UNSUPPORTED: !fir
+// XFAIL: *
 
----------------
You're disabling the test here, what's happening?


================
Comment at: flang/tools/tco/tco.cpp:64
   mlir::MLIRContext context;
-  fir::registerFIRDialects(context.getDialectRegistry());
   auto owningRef = mlir::parseSourceFile(sourceMgr, &context);
----------------
I'm surprised you don't need this, the call to parseSourceFile right below should rely on dialects being registered?


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

https://reviews.llvm.org/D95399