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

Wed Sep 21 07:31:37 PDT 2022

jeanPerier added a comment.

Thanks for the review and feedback @peixin.

================
Comment at: flang/docs/HighLevelFIR.md:86
+  assignments or transformational intrinsics).
+- Generating debug information for the variables based on the fir.declare
+  operation.
----------------
peixin wrote:
> Will the debug information be `mlir::Location`?
No, I believe source line location is already making it into the executable. What we are currently missing is the ability to interact with Fortran source variables from a debugger (e.g print, set, or watch a variable). I am not sure exactly how and when the related DWARF will have to be generated. This is a big task I should add on our TODO list. The point here is that fir.declare should allow generating this information late, because it documents everything that needs to be known about a source variable (address or descriptor, bounds, type parameters).

================
Comment at: flang/docs/HighLevelFIR.md:88
+  operation.
+- Generic Fortran aliasing analysis (currently implemented only around  array
+  assignments with the fir.array_load concept).
----------------
peixin wrote:
> peixin wrote:
> > nit
> Will this include the aliasing analysis in Fortran 2018 15.5.2.13? Is this `Generic Fortran aliasing analysis` documented somewhere with examples currently?
Yes, that is the idea. And no, this is not documented yet. This would deserve its own document. The idea here is that when seeing an address in FIR, we will try to find a matching fir.declare or fir.associate node. If it is the parent operation, we will directly have all the Fortran information that allows applying 15.5.2.13. If it is a block argument, the analysis will try to follow the block argument predecessors if it can, and create a list of possible Fortran variables the address belongs to. If such list cannot be built, the analysis will have conservatively to assume the address may overlap with anything.

================
Comment at: flang/docs/HighLevelFIR.md:136
+The proposal is to add a !fir.expr<T> SSA value type concept, and set of
+character operations (concatenation, TRIM, MAX, MIN, ...), a set of array
+transformational operations (SUM, MATMUL, TRANSPOSE, ...), and a generic
----------------
peixin wrote:
> Can `COMPARE` be added in the set of character operations? We found inlining character comparison has great performance improvement in some workloads (the possible reason is the character is in select case statement). Adding it will make inline work easier?
Yes, I had it in mind in the "..." parts. I made it explicit.

================
Comment at: flang/docs/HighLevelFIR.md:169
+
+Motivation: avoid the need to materialize expressions in temporaries while
+lowering.
----------------
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.

================
Comment at: flang/docs/HighLevelFIR.md:195
+-   A floating point type (f32, f16…)
+-   A complex type (fir.complex<4> or mlir::complex<f32>...)
+
----------------
peixin wrote:
> Why keep two complex types instead of only using fir.complex or mlir::complex?
> Why keep two complex types instead of only using fir.complex or mlir::complex?

The need arise from the fact that we want to make a difference between C/Fortran complex type, and C++ std::complex type. These types are layout compatible, but not ABI compatible (they are not passed the same way on all architectures). fir::Complex implements C/Fortran complex, and mlir::Complex is translated to an std::complex  like struct.

In the runtime API, we sometime interface with std::complex<>.

Using distinct types to enforce the ABI may not be the only solution. But it was the simplest and more robust so far (especially given we already had both types). If a solution is found regarding this ABI problem, moving to using mlir::complex would sound OK to me. I see it as something orthogonal to these high level changes (it could be done in parallel). The solution is probably to translate std::complex to a struct type and consider mlir::Complex to be the C complex from an ABI point of view (but I think this may conflicts with MLIR current codegen towards LLVM that would have to be overridden). fir.convert between mlir complex and the related struct type would have to be allowed and probably implemented by going through memory and doing the cast there to rely on the layout compatibility property.

================
Comment at: flang/docs/HighLevelFIR.md:211
+- %element = fir.apply %expr, %i, %j : (!fir.expr<T>, index index) ->
+  fir.expr<ScalarType> |  AnyConstantSizeScalarType
+
----------------
peixin wrote:
> editor problem?
Yes, thanks. I replaced all the non breaking spaces by normal spaces and remove the redundant ones like here.

================
Comment at: flang/docs/HighLevelFIR.md:226
+most often not be a block argument. This is because the fir.expr is mostly
+intended to allow combining chains of operations into more optimal forms. But it
+is possible to represent any expression result via a Fortran runtime descriptor
----------------
peixin wrote:
> This should be added in the description when this opertion is added in FIROps.td.
> 
> I am thinking if `fix.expr<T>` is a good solution for the argument with VALUE attribute in lowering (both caller and callee side), especially for the derived type and BIND(C)?
> I am thinking if fix.expr<T> is a good solution for the argument with VALUE attribute in lowering (both caller and callee side), especially for the derived type and BIND(C)?

It may help, but since this type is not intended to survive until LLVM codegen, the VALUE aspect would still have to be translated with the current FIR types as it is currently.

================
Comment at: flang/docs/HighLevelFIR.md:266
+
+fir.declare will be used for all Fortran variables, except the ones created via
+the ASSOCIATE construct that will use fir.associate described below.
----------------
peixin wrote:
> Will `fir.declare` be used for common blocks as well?
It will be used for the common block members once the address computation is done (maybe with the common block name as a fir.common attribute so that it can easily be identified which common block a variable belongs to). Same idea with equivalences. I am not sure if there is a need to declare the whole common block in a declare op, but if it turns out useful for OpenMP or other usages, why not.

================
Comment at: flang/docs/HighLevelFIR.md:780
+-   On top of the above points, Forall can contain user assignments, pointer
+    assignments, and assignment to whole allocatable.
+
----------------
peixin wrote:
> Another tough: The mask expression lhs variable might be changed in the assignment. There seems to be one bug in current FIR lowering. fir.forall may make it easy to support that case.
Thanks, indeed. I added  note about masks

================
Comment at: flang/docs/HighLevelFIR.md:792
+    assignments. Any assignments syntactically before the where mask occurrence
+must be performed before the mask evaluation.
+
----------------
peixin wrote:
> ?
Yes, I agree the mask evaluation cannot affect anything (just like the forall indices evaluation), but the mask evaluation may be affected by the assignments.

================
Comment at: flang/docs/HighLevelFIR.md:969
+subroutine foo(a, b)
+   real :: a(:), b(:)
+   a = b
----------------
peixin wrote:
> Same for line 970, 978-981, 992-1007, 1032-1041, 1053-1056, ...
Thanks, all the none breaking space issues should have been fixed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285