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

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

In D134285#3845772 <https://reviews.llvm.org/D134285#3845772>, @kiranchandramohan wrote:

> Thanks @jeanPerier for this RFC. Quite detailed, informative and looks good. I had a quick read and have a few questions or comments inline.
>
> I also have the following general questions,

Thanks for the review @kiranchandramohan !

> -> Do you see any issues in FIR/MLIR optimisations due to the presence of `fir.declare`?

I think it will help building alias analysis for FIR and that will help optimizations in general. It can be labeled as having no side effects, so in it should not disturb passes that do not care much about address origins. But you are raising a good point that mem2reg will have to do something about it.

> -> Would there be scope for the community to get involved in this work to speed it up?

Yes, I think help will be welcomed. I am trying to set-up some skeleton for this new flow. And then, I think some help will be welcomed, especially around the new FIR character ops and intrinsics.

> -> Would it be better to delay any work on emitting debug info till HighLevelFIR work is complete?

No, if anyone wants to jump on debug info, please do, bearing in mind that fir.declare should be a good fit to extract the required info. I will try to add this op early if anyone wants to work on this. I anyway think the first piece of debug info will be to work a small document to summarize what LLVM needs, and give a plan.



================
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.
----------------
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.


================
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.
----------------
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.


================
Comment at: flang/docs/HighLevelFIR.md:441
+
+-   realloc: mark that assignment has F2003 semantics and that the left-hand
+    side may have to be deallocated/reallocated…
----------------
kiranchandramohan wrote:
> Will there be `fir.allocate` or `fir.deallocate` operations generated for these?
Not until alias analysis is done. The rational is that the reallocation can be optimized when there is a potential overlap (the temp can be moved to the new allocatable value).

So lowering will only add the realloc attribute, and a FIR to FIR pass will lower this to a set of  fir.assign (without the realloc attribute) + fir.allocate + fir.deallocate + necessary fir.if nests taking advantage of alias analysis.


================
Comment at: flang/docs/HighLevelFIR.md:484
+Motivation: keep POINTER and ALLOCATABLE allocation explicit in FIR, while
+allowing later lowering to Fortran runtime calls to allow the runtime to do
+Fortran specific bookkeeping or flagging of allocations.
----------------
kiranchandramohan wrote:
> Will `fir.allocate` lower to `fir.allocmem` or to runtime calls? Why?
> Will fir.allocate lower to fir.allocmem or to runtime calls

Currently, the runtime is only used when allocations are none trivial (include initializations, polymorphic types, or error handling). However, we may want to offer the user the ability to debug their allocations/deallocation by validating that ALLOCATABLE being deallocated by have indeed been allocated, and are not pointing to bad addresses. In those case, the runtime would always be used.

In general, users may like the ability too easily hook into Fortran allocatable/pointer allocation via the runtime without recompiling programs. Some may prefer to have all allocations inlined when possible. So I think having the option in fir.allocate/fir.deallocate between fully inlined allocation with fir.allocmem or using the runtime makes sense.

I edited the sentence to make this clearer.


================
Comment at: flang/docs/HighLevelFIR.md:536
+Note that %indices are not operands, they are the elemental region block
+arguments, representing the array iteration space in a one based fashion.
+
----------------
kiranchandramohan wrote:
> Nit: Could you add some explanation for `one based`?
> Nit: Could you add some explanation for `one based`?

Sure, I added a note. The purpose if to match Fortran default for array variables so that there is no need to generate bound adjustments when working with one based array variables in an expression.


================
Comment at: flang/docs/HighLevelFIR.md:623
+```
+%res = fir."intrinsic_name" %expr_or_var, ...
+```
----------------
kiranchandramohan wrote:
> I am assuming the simplification of intrinsics pass (with minor modifications) will still be useful for generating an inlined version. Or would lowering and high-level FIR make this redundant?
> 
> I am assuming the simplification of intrinsics pass (with minor modifications) will still be useful for generating an inlined version.

Absolutely, the plan is to use this pass (modifying it so that it can tape into the fir.intrinsic ops ).


================
Comment at: flang/docs/HighLevelFIR.md:687
+
+Having a representation to represent array constructor will allow an easier
+lowering of array constructor, and make array ctor a lot easier to manipulate.
----------------
kiranchandramohan wrote:
> Would this have a lowering to `fir.allocmem`?
It depends of the array constructor.
I think we should not generate a fir.allocmem for small and easy things like `[%i, %j, %k]` (only scalar, no ac-implied, less than N elements, where N is some option). This should lower to fir.alloca.
Otherwise, yes, fir.allocmem will be used, although reallocation will still be needed to deal with the edge cases where the final size cannot be pre-computed (for instance `[foo(), bar(), buzz()]`, where all three functions returns rank-one allocatable arrays).


================
Comment at: flang/docs/HighLevelFIR.md:1076
+    %belt = fir.designate %b, %i {fir.ref = "_QPfooEb", fir.def= "_QPfooEb.des001"}
+    %p_lb = fir.get_lbound %p {fir.ref = "_QPfooEp"}
+    %i_zero = arith.subi %i, %c1
----------------
kiranchandramohan wrote:
> Is `fir.get_lbound` a new operation?
Thanks for catching this inconsistency. When writing FIR manually, I found it useful to avoid dealing with the details around pointers and allocatable (fir.load %box + fir.bo_dims). But it is not a game changer, and I am still debating whether this is useful or not.

I added a description for it above assuming it will be added for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285