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

[Kiran Chandramohan via Phabricator via flang-commits]

kiranchandramohan added a comment.

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,
-> Do you see any issues in FIR/MLIR optimisations due to the presence of `fir.declare`?
-> Would there be scope for the community to get involved in this work to speed it up?
-> Would it be better to delay any work on emitting debug info till HighLevelFIR work is complete?



================
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.
----------------
Till when will these `fir.declare`'s persist?  Will this have an effect on optimisations particularly the mem2reg kind of transformations.


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


================
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…
----------------
Will there be `fir.allocate` or `fir.deallocate` operations generated for these?


================
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.
----------------
Will `fir.allocate` lower to `fir.allocmem` or to runtime calls? Why?


================
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.
+
----------------
Nit: Could you add some explanation for `one based`?


================
Comment at: flang/docs/HighLevelFIR.md:582
+This is the addressing equivalent for expressions. A notable difference is that
+it cannot only take simple scalar indices (no triplets) because it is not clear
+why supporting triplets would be needed, and keeping the indexing simple makes
----------------



================
Comment at: flang/docs/HighLevelFIR.md:623
+```
+%res = fir."intrinsic_name" %expr_or_var, ...
+```
----------------
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?



================
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.
----------------
Would this have a lowering to `fir.allocmem`?


================
Comment at: flang/docs/HighLevelFIR.md:934
+     expression concepts (requires 1.).
+- 3. Intoduces the new translation passes, using the fir::ExtendedValue helpers
+     (requires 1.).
----------------



================
Comment at: flang/docs/HighLevelFIR.md:1042
+    %bval = fir.load %belt : f32
+    fir.store %bval to %alet : fir.ref<f32>
+  }
----------------



================
Comment at: flang/docs/HighLevelFIR.md:1055-1060
+subroutine foo(a, b, p, c)
+  real.  target :: a(:)
+  real : b(:), d(100)
+  real, pointer :: p(:)
+  a = b*p + c
+end subroutine
----------------



================
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
----------------
Is `fir.get_lbound` a new operation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134285