[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#3849053 <https://reviews.llvm.org/D134285#3849053>, @kiranchandramohan wrote:

> 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?

Yes, HLFIR will work with the existing FIR types (it has to, since it is not adding any types for variables). And appart from the hlfir.forall, hlfir doe not contain any construct like operation, it is all about expressions and assignments, so it lowering will generate FIR + HLFIR.



================
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
----------------
kiranchandramohan wrote:
> 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.
In the symbol table in lowering, it will be bound to the fir.declare result.
The goal is that it is possible to emit high level operation (e.g. hlfir.assign) with the SSA value bound to front-end symbol.

Regarding `createHostAssociateVarClone`, yes, it might need to emit a new fir.declare so that the new entity can be properly binded.


================
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:
> 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.
> I am assuming there will be changes required for our MemRefDataFlowOpt pass to account for the fir.declare operation.

Yes, that seems right to me. I do not think we are running this pass currently. Regardless, fir.declare should allow getting a higher level view in possible aliasing/side-effects affecting an address, and should help getting the store to load forwarding working again safely.

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

Thanks for this pointer. I see fir.declare as being a bit different than llvm.dbg.declare in the sense that fir.declare will give context to its output address, but it will have no impact on the semantics of its input.

You could very well imagine implementing storage sharing with fir.declare:

```
subroutine foo()
  integer :: i, j
  ! some code using i but not j

  ! some code using j but not i
end subroutine
```

After some optimization pass, you could have:
```
func.func @_QPfoo() {
  %storage = fir.alloca i32
  %i = fir.declare %stoarge {fir.def = _QPfooEi}
  %j = fir.declare %stoarge {fir.def = _QPfooEj}
   // ... use %i
   // ... use %j
}
```

llvm.dbg.declare also seems to require that there can only be one per variable. Which makes some optimization where a variable may live in different storage/register during its lifetime harder.

I do not think the same will be true of fir.declare. It requires its fir.def tag to be uniq so that the defining op is always accessible in hlfir ops using it, but a same tag could point to the same Fortran variable.
You could have:
```
%storage1 = fir.alloca i32
%i_1 = fir.declare %storage  {fir.def = _QPfooEi}
// use %i_1.... debugger should be told "i" is in %i_1
// for some reason decide that "i" is now tracked as an element in some buffer:
%buffer = fir.alloca i32, 10
%storage2 = fir.coordinate_of %buffer, c3
 %i_2 = fir.declare %storage  {fir.def = _QPfooEi.opt1}
// use %i_2. debugger should be told "i" is in %i_2
```

I think this would translate in llvm.dg.addr being emitted when changing the storage, but this deserves more design work about debug info generation.

At least from an HLFIR/FIR point of view, I see no issue with the two patterns above (two fir.declare sharing the same input, or two fir.declare referring to the same Fortran variable). I think fir.declare, despite the name, is more akin to llvm.dbg.addr, and that one big advantage it has over both is that it returns an SSA value, while the other are tagging an SSA existing value. That makes variable info identifiable via the defining operations chain of an address, rather than via an analysis of its usages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285