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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz added inline comments.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRAttr.h:137
+/// underlying type of the pointee object is left up to the client. Opaque
+/// attributes are always constructed as null pointers when parsing.
+class OpaqueAttr
----------------
mehdi_amini wrote:
> I'm not sure how it works? How is the context reconstructed on reparsing these?
> 
> Also I don't see any test using such attribute right now, can you provide one?
This attribute allows us to attach a dictionary of on-the-side data structures to a ModuleOp. These data structures do not change the meaning of the FIR/MLIR representation but may be convenient in other ways.

For the sake of roundtripping, the exact nature of the objects being referenced is washed out. They effectively are optional attributes that roundtrip as "None" values.

We can certainly add a test.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRDialect.h:68
   // clang-format on
+  registry.loadAll(&ctx);
 }
----------------
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.



================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1107
+  let assemblyFormat = [{
+    $memref (`(` $shape^ `)`)? (`[` $slice^ `]`)? (`typeparams` $lenParams^)? (`map` $accessMap^)? attr-dict `:` functional-type(operands, results)
   }];
----------------
mehdi_amini wrote:
> It'd be nice to have at least one test that parses all the fields as an example, and have it in the description here in ODS as well.
Agreed. That was the case, but as FIR has been developing and changing, some of these new options and ops should be added.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1149
     }
     return mlir::success();
   }];
----------------
mehdi_amini wrote:
> That's quite some C++ code in ODS. In general for non trivial method please replace with a call to a function implemented in the .cpp file: it plays much nicer with IDEs and debuggers.
> (same below for the new ops you're introducing)
No problem.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1488
+      %d = fir.shape(%c_100) : (index) -> !fir.shape<1>
+      %b = fir.embox %r shape %d : (!fir.ref<i64>, !fir.shape<1>) -> !fir.box<i64>
       %a = fir.box_isarray %b : (!fir.box<i64>) -> i1  // true
----------------
mehdi_amini wrote:
> Is the "shape" keyword a valid syntax? I'm not sure I understand from the declarative syntax above
> 
No, it is not. Thanks for pointing this out. Looks like a typo or 3 here.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1945
+            fir::isa_char_string(eleTy)))
+        return emitOpError("cannot apply coordinate_of to this type");
     }
----------------
mehdi_amini wrote:
> Are there tests for all these diagnostics?
We are working on a set of negative diagnostic tests and will add them.


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:590
+// These patterns are written by hand because the tablegen pattern language
+// isn't adequate here.
+template <typename FltOp, typename CpxOp>
----------------
mehdi_amini wrote:
> I'd rather read a comment that just describes what the pattern is doing on a high level / a snippet of the replacement.
Sure. We can clean this up, make it clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95399