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

[Kiran Chandramohan via Phabricator via flang-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/docs/HighLevelFIR.md:21
+MLIR translation passes.  As a result of these additions, it is likely that the
+fir.array_load/fir.array_merge_store and related array operations could be
+removed from FIR since array assignment analysis could directly happen on the
----------------
And the `ArrayValueCopy` pass will not be needed?


================
Comment at: flang/docs/HighLevelFIR.md:83
+
+The fir.declare operation will allow:
+- Pushing higher-level Fortran concepts into FIR operations (like array
----------------
>From the OpenMP side, in the long-term we would like to move privatisation into the OpenMP dialect. So it will be very helpful if variable declarations are available in FIR. On the other hand, having more kinds of variables in Flang (FIR, HighFIR) would mean that we will have to get OpenMP Dialect to work with these different kinds of variables.

Can you consider adding the `finalizer` also to the declare operation? 


================
Comment at: flang/docs/HighLevelFIR.md:169
+
+Motivation: avoid the need to materialize expressions in temporaries while
+lowering.
----------------
jeanPerier wrote:
> peixin wrote:
> > Currently, OpenMP reduction clause uses the fragile pattern match in lowering. When refactoring the lowering code, is it possible to make some expressions lowering shared with OpenMP using `fir.expr`? Specifically, the following operators, intrinsics, and user-defined operators will be redefined in OpenMP reduction clause:
> > ```
> > +, *, .and., .or., .eqv., .neqv., max, min, iand, ior, ieor
> > ```
> Interesting, what are you currently pattern matching ?
> I am not sure the current design would help you a lot in the sense that it intends to not avoid any specific elemental operations, but rather to use a generic fir.elemental concepts.
> 
> As much as possible, I would tend to extend the mlir arith dialect to support the scalar operations applicable on MLIR integer and floating point types and pattern match that. But then, for some elemental operators  and intrinsics for which easy pattern matching is desirable, and where adding an arith op is not the best way, it could makes sense to add custom ops. But this may be orthogonal to the proposed change and something that could be though/designed/done in parallel, operators by operators.
> 
> Regarding user defined operators, I would need to understand better what you mean by "redefined", semantics is resolving user defined operation to function references already, so we do not really see them in lowering I believe.
Thanks @peixin for raising this point.

Currently we do a pattern match fir loads -> (optional convert) -> reduction operation in FIR/arith -> (optional convert) -> store and replace it with an `omp.reduction ` operation.

So for a reduction happening on an addition (say `b = b + a`).
We pattern match on the following IR,
```
%b = fir.load %bref
%a = fir.load %aref
%r = arith.add %a, %b
fir.store %r
```
and get the following result IR.
```
%a = fir.load %aref
omp.reduction %a, %bref
```

An alternative approach that we are exploring is to see whether this can be done directly during lowering, i.e. to do custom lowering for the Assignment operation if it happens to perform a reduction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285