[flang-commits] [PATCH] D137634: [flang][RFC] Do not rely on attributes to tag HLFIR variable uses

Tue Nov 8 07:56:57 PST 2022

PeteSteinfeld accepted this revision.
PeteSteinfeld added inline comments.
This revision is now accepted and ready to land.

================
Comment at: flang/docs/HighLevelFIR.md:91
+This creates a split of the HFLIR variables in two categories: the ones for
+which the defining operation is retrievable, that will be called
+`FortranVariable`, and the ones for which the defining operation is not
----------------
Should read "are retrievable".

================
Comment at: flang/docs/HighLevelFIR.md:92
+which the defining operation is retrievable, that will be called
+`FortranVariable`, and the ones for which the defining operation is not
+retrievable, that will be called `FortranLikeVariable`. Lowering to HLFIR,
----------------
Should read "are not".

================
Comment at: flang/docs/HighLevelFIR.md:97
+transformation passes will be required to be able to operate on
+`FortranLikeVariable`. This will gives more freedom for optimizations to clone,
+move and merge the code without creating situations where HLFIR code generation
----------------
Shoud read "This will give".

================
Comment at: flang/docs/HighLevelFIR.md:108-113
+In practice, it is expected that the passes will be able to leverage
+hlfir.declare in most cases, but that guaranteeing that it will always be the
+case would constraint the IR and optimizations too much.  The goal is also to
+remove the fir.box usages when possible while lowering to FIR, to avoid
+needlessly creating runtime descriptors for variables that do not really
+require it.
----------------
This all sounds complicated.  Having two kinds of variable, sometimes including attributes and sometimes not.  I'm not sure what the advantage of this is or if there's a simpler way to accomplish what you're trying to do.

================
Comment at: flang/docs/HighLevelFIR.md:124
+The hlfir.declare will have a sibling fir.declare operation in FIR that will
+allow keeping variable information until debug info are generated. The main
+difference is that the fir.declare will simply return its first operand,
----------------
Should read "is generated".

================
Comment at: flang/docs/HighLevelFIR.md:294
+will have the same type as %base, it is intended to be used when lowering HLFIR
+to FIR in order to avoid creating unnecessary fir.box (that would became
+runtime descriptors). When an HLFIR operation has access to the defining
----------------
Should read "would become"

================
Comment at: flang/docs/HighLevelFIR.md:1346-1348
+Instead of introducing hlfir.declare, restricting what memory types an HLFIR
+variable it was considered to force the defining operation of HLFIR variable
+SSA values to always be retrievable. The idea was to add a fir.ref attribute
----------------
I couldn't quite parse this sentence.  Did you mean to say `Instead of introducing an hlfir.declare operation, which would restrict the memory types for an HLFIR variable, it was considered to force ...`?

================
Comment at: flang/docs/HighLevelFIR.md:1349-1350
+SSA values to always be retrievable. The idea was to add a fir.ref attribute
+that would repeat the name of the HLFIR variable. Using such attribute would
+prevent MLIR to merge to operations using different variable in a new block
+(which is the main reason why the defining op may become inaccessible).
----------------
Reword to `Using such an attribute would prevent MLIR from merging to operations using a different variable in a new block...`?

================
Comment at: flang/docs/HighLevelFIR.md:1352
+(which is the main reason why the defining op may become inaccessible).
+The advantage is that it allowed all Fortran information of variables (like
+attributes) to always be accessible in HLFIR when looking at their uses, and
----------------
I would say `The advantage of forcing the defining operation to be retrievable is that ...`

================
Comment at: flang/docs/HighLevelFIR.md:1354
+attributes) to always be accessible in HLFIR when looking at their uses, and
+avoid requiring introducing fir.box usages for simply contiguous variables.
+The big drawback is that it implied naming all HLFIR variables, that cover
----------------
Should read "avoids requiring the introduction of fir.box ..."

================
Comment at: flang/docs/HighLevelFIR.md:1355-1356
+avoid requiring introducing fir.box usages for simply contiguous variables.
+The big drawback is that it implied naming all HLFIR variables, that cover
+much more than Fortran named variables. Naming designators with unique names
+was not very natural, and would make designator CSE harder.  It also made
----------------
Do you mean that there will be many more HLFIR variables than Fortran variables?  If so, perhaps reword as `The big drawback is that this implies naming all HLFIR variables, and there are many more of them than there are Fortran named variables.`

================
Comment at: flang/docs/HighLevelFIR.md:1360
+attributes renaming would break the name uniqueness, which could lead to some
+operation using different variables to be merged, and to break the assumption
+that parent operation must be visible. Renaming would be possible, but would
----------------
Should read "operations"

================
Comment at: flang/docs/HighLevelFIR.md:1361
+operation using different variables to be merged, and to break the assumption
+that parent operation must be visible. Renaming would be possible, but would
+increase complexity and risks. Besides, inlining may not be the only
----------------
Should read "operations".

================
Comment at: flang/docs/HighLevelFIR.md:1373
+and would require to turn every FIR operation with a region into an MLIR symbol
+table. This would especially be annoying given fir.designator also produce
+variables with their own properties, which would imply creating a lot of MLIR
----------------
Should read "annoying since fir.designator also produces"

================
Comment at: flang/docs/HighLevelFIR.md:1379
+turn a variable into an expression). Given all variable definitions will
+dominates their uses, it seems more adequate to use an SSA model with named
+attributes. Using SSA values also makes the transition and mix with lower-level
----------------
Should read "dominate their uses, it seems better to use ..."

================
Comment at: flang/docs/HighLevelFIR.md:1380
+dominates their uses, it seems more adequate to use an SSA model with named
+attributes. Using SSA values also makes the transition and mix with lower-level
+FIR operations smoother: a variable SSA usage can simply be replaced by
----------------
Should read "and mixture with"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137634