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

Wed Sep 21 02:19:20 PDT 2022

peixin added a comment.

Very good design. This seems to be one promising method to handle some bugs in forall, pattern match for inline intrinsics and OpenMP reduction (not totally sure if it will work), and expected to have more information of variables and expression for performance optimization in FIR.

================
Comment at: flang/docs/HighLevelFIR.md:74
+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
+"fir.def = uniq_mangled_variable_name" that will allow linking it to the Fortran
----------------
Nit

================
Comment at: flang/docs/HighLevelFIR.md:86
+  assignments or transformational intrinsics).
+- Generating debug information for the variables based on the fir.declare
+  operation.
----------------
Will the debug information be `mlir::Location`?

================
Comment at: flang/docs/HighLevelFIR.md:88
+  operation.
+- Generic Fortran aliasing analysis (currently implemented only around  array
+  assignments with the fir.array_load concept).
----------------
nit

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

================
Comment at: flang/docs/HighLevelFIR.md:93
+the result of function references returning POINTERs. fir.declare will also
+accept such variables to be described in the IR (a uniq name will be built from
+the caller scope name and the function name.). In general, fir.declare will
----------------
Nit

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

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

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

================
Comment at: flang/docs/HighLevelFIR.md:211
+- %element = fir.apply %expr, %i, %j : (!fir.expr<T>, index index) ->
+  fir.expr<ScalarType> |  AnyConstantSizeScalarType
+
----------------
editor problem?

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

================
Comment at: flang/docs/HighLevelFIR.md:229
+(fir.box<T>), implying that if a fir.expr<T> is passed as a block argument, the
+expression bufferization pass will  evaluate the operation producing the
+expression in a temporary, and transform the block operand into a fir.box
----------------

================
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.
----------------
Will `fir.declare` be used for common blocks as well?

================
Comment at: flang/docs/HighLevelFIR.md:309
+the variable locally, and to encode certain side-effects (like copy-in/copy-out
+when going from a non-contiguous  variable to a contiguous variable, with the
+help of the related fir.end_association operation).
----------------

================
Comment at: flang/docs/HighLevelFIR.md:370
+a fir.allocmem, lowering should insert a fir.freemem after the fir.finalize.
+This to help making fir.allocmem to fir.alloca promotion simpler, and also
+because finalization may be run without the intent to deallocate the variable
----------------
`to help` -> `help`?

================
Comment at: flang/docs/HighLevelFIR.md:436
+```
+fir.assign %expr_or_var  to %var [attributes]
+```
----------------
editor problem?

================
Comment at: flang/docs/HighLevelFIR.md:460
+the right hand side was always evaluated in a temporary. The motivation to use
+fir.assign is to help the temporary removal, and also  to deal with two edge
+cases: user assignment in a FORALL (the forall pass will need to understand that
----------------

================
Comment at: flang/docs/HighLevelFIR.md:529
+%op = fir.elemental (%indices) %shape [%type_params] [%dynamic_type] {
+   ….
+  fir.result %result_element
----------------
editor problem?

================
Comment at: flang/docs/HighLevelFIR.md:530
+   ….
+  fir.result %result_element
+}
----------------
editor problem?

================
Comment at: flang/docs/HighLevelFIR.md:541
+```
+%add = fir.elemental (%i:index, %j:index) shape %shape (!fir.shape<2>) -> !fir.expr<?x?xf32>> {
+  %belt = fir.designate %b, %i, %j {fir.ref = _QPfooEb, fir.def = _QPfooEb.des001}: (!fir.ref<!fir.array<?x?xf32>>, index, index) -> !fir.ref<f32>
----------------
nit

================
Comment at: flang/docs/HighLevelFIR.md:544
+  %celt = fir.designate %c, %i, %j {fir.ref = _QPfooEa, fir.def = _QPfooEa.des002} : (!fir.ref<!fir.array<?x?xf32>>, index, index) -> !fir.ref<f32>
+  %bval = fir.load  %belt : (!fir.ref<f32>) -> f32
+  %cval = fir.load  %celt : (!fir.ref<f32>) -> f32
----------------
It seems there is some editor problem? from line 542 to line 547.

================
Comment at: flang/docs/HighLevelFIR.md:698
+
+Note that fir.elemental could be used to implement some ac-implied-do, although
+this is not yet clarified since ac-implied-do may contain more than one scalar
----------------
?

================
Comment at: flang/docs/HighLevelFIR.md:750
+Motivation: represent conformity requirement/information between two array
+operands so that later optimization can chose the best shape information source,
+or insert conformity runtime checks.
----------------

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

================
Comment at: flang/docs/HighLevelFIR.md:792
+    assignments. Any assignments syntactically before the where mask occurrence
+must be performed before the mask evaluation.
+
----------------
?

================
Comment at: flang/docs/HighLevelFIR.md:808
+      evaluating previous assignments)
+-   3. For each assignments, check if the RHS/LHS operands value may depend
+     on the LHS base:
----------------

================
Comment at: flang/docs/HighLevelFIR.md:881
+    fir.expr, and all the operations that may produce it like transformational
+intrinsics and fir.elemental, fir.apply).
+-   Call interface argument association lowering (getting rid of fir.associate
----------------

================
Comment at: flang/docs/HighLevelFIR.md:969
+subroutine foo(a, b)
+   real :: a(:), b(:)
+   a = b
----------------
Same for line 970, 978-981, 992-1007, 1032-1041, 1053-1056, ...

================
Comment at: flang/docs/HighLevelFIR.md:1139
+-   Insert temporary, and duplicate array assignments, that can be lowered to
+    loops at that point
+
----------------

================
Comment at: flang/docs/HighLevelFIR.md:1330
+
+Issues: 
+
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285