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

[Jean Perier via Phabricator via flang-commits]

jeanPerier marked an inline comment as done.
jeanPerier added a comment.

Thanks @PeteSteinfeld for the detailed review !



================
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.
----------------
PeteSteinfeld wrote:
> 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.
Yes, I removed this paragraph. It is too much details not really needed here. What I was saying is that when you look at an HLFIR variables, you will either be able to know about its Fortran attributes, or you will not. In lowering (when first generating the HLFIR from the parse tree) we will always be able to know about attributes, because no optimization passes may have "hidden" the source yet, so it is OK to rely on having access to it at that stage. Then, in HLFIR to HLFIR passes, we may no longer have access to it and we should not rely on having access to it for correctness purposes.


================
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
----------------
PeteSteinfeld wrote:
> 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 ...`?
Changed to `Instead of restricting ....` the hlfir.declare op is not very important here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137634