[flang-commits] [PATCH] D134285: [flang][RFC] Adding higher level FIR ops to ease expression lowering

[Kiran Chandramohan via Phabricator via flang-commits]

kiranchandramohan added a comment.

Thanks @jeanPerier for the quick reply. A couple of comments or questions inline.

So  I guess `hlfir.expr<T>` type is the only new type and is expected to work only with `hlfir`. Otherwise `hlfir` is expected to work with existing `fir` types?



================
Comment at: flang/docs/HighLevelFIR.md:73
+The proposal is to add a fir.declare operation that would anchor the
+fir::ExtendedValue information in the IR regardless of the mlir::Value used for
+the variable (bare memory reference, or fir.box). This operation will have a
----------------
What will the frontend symbol be bound to after this change? Will it be the value generated by `fir.declare` or the `fir.alloca` ? I guess we might need some changes for the `createHostAssociateVarClone` function.


================
Comment at: flang/docs/HighLevelFIR.md:95
+the caller scope name and the function name.). In general, fir.declare will
+allow to view every memory storage as a variable, and this will be used to
+describe and use compiler created array temporaries.
----------------
jeanPerier wrote:
> kiranchandramohan wrote:
> > Till when will these `fir.declare`'s persist?  Will this have an effect on optimisations particularly the mem2reg kind of transformations.
> My plan was for fir.declare to persist until LLVM codgen since it is a no-op that only carries information. But you are raising a good point about mem2reg. Do you know if there is a generic MLIR mem2reg pass  ? In any case, I expect mem2Reg pass might need to be aware of the fir.declare "transparent" aspect and be able to delete it for variables promoted to SSA value (and move its attributes somewhere if debug info is still needed for the variable ? I am not familiar enough with debug info to be assertive here).
> 
> Regarding optimization in general, I think keeping it is in our interest since it will allow to document Fortran aspects about attributes (like target vs not target) even after inlining, which should allow better alias analysis.
I don't think there is currently a mem2reg pass in MLIR. But there are a few downstream projects which have this and there were a couple of discussions talking about upstreaming this. But I don't think this has happened yet.
https://discourse.llvm.org/t/rfc-store-to-load-forwarding/59672
https://discourse.llvm.org/t/upstreaming-from-our-mlir-python-compiler-project/64931/4

I am assuming there will be changes required for our `MemRefDataFlowOpt` pass to account for the `fir.declare` operation.

I am also reminded of llvm's `dbg intrinsics` (https://llvm.org/docs/SourceLevelDebugging.html#id13). These intrinsics track variable information and includes a `llvm.dbg.declare` intrinsic which is deprecated. Not sure whether `fir.declare` will also face similar issues.


================
Comment at: flang/docs/HighLevelFIR.md:315
+
+A fir.associate must be followed by a related fir.end_association that will
+allow inserting any necessary finalization or copy-out later.
----------------
jeanPerier wrote:
> kiranchandramohan wrote:
> > Can this be modeled by a region instead?
> > 
> > We discussed this in the Flang technical call. The reasons mentioned include difficulty handling exits, branches out of this etc.
> One of the main issue I think is that it is intended to deal with argument association on the caller sides. So if we used regions, you would have something like:
> 
> fir.associate %actual1 to %dummy1 {
>   fir.associate %actual2 to %dummy2 {
>     ....
>     %res = fir.call %foo(%dumm1, %dummy2)
>     ...
>   }
> }
> 
> The first issue here is that %res is not accessible after the call where it needs to be used. It could be propagated back, but in general, this means that any SSA values created during the association lifetime is unusable, even if it is not linked to toe variable lifetime. And I find this very restrictive hard to work with.
> 
> Then, nested regions are harder to reorder than operations in a same block. Fortran tells arguments can be evaluated in any order. But generating such associate nest in lowering would in my opinion make it harder to re-order things here to to better CSE for instance.
> The first issue here is that %res is not accessible after the call where it needs to be used. It could be propagated back, but in general, this means that any SSA values created during the association lifetime is unusable, even if it is not linked to toe variable lifetime. And I find this very restrictive hard to work with.

Yes, that is right particularly after `mem2reg` kind of passes. But for load-store kind of code that lowering generates, it should not be a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285