This is an archive of the discontinued LLVM Phabricator instance.

[flang][RFC] Do not rely on attributes to tag HLFIR variable uses
ClosedPublic

Authored by jeanPerier on Nov 8 2022, 5:20 AM.

Details

Summary

After more considerations and experience, switch to one of the
alternative plan for HLFIR variable that will avoid requiring naming
designators and having to maintain and update names in attributes after
inlining of code duplication.

The cost is the increase of fir.box usage, which in most cases should
be removed when lowering from HLFIR to FIR.

Diff Detail

Event Timeline

jeanPerier created this revision.Nov 8 2022, 5:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 5:20 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
jeanPerier requested review of this revision.Nov 8 2022, 5:20 AM
PeteSteinfeld accepted this revision.Nov 8 2022, 7:56 AM
PeteSteinfeld added inline comments.
flang/docs/HighLevelFIR.md
91

Should read "are retrievable".

92

Should read "are not".

97

Shoud read "This will give".

108–113

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.

115

Should read "is generated".

285

Should read "would become"

1337–1339

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

1340–1341

Reword to Using such an attribute would prevent MLIR from merging to operations using a different variable in a new block...?

1343

I would say The advantage of forcing the defining operation to be retrievable is that ...

1345

Should read "avoids requiring the introduction of fir.box ..."

1346–1347

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.

1351

Should read "operations"

1352

Should read "operations".

1364

Should read "annoying since fir.designator also produces"

1370

Should read "dominate their uses, it seems better to use ..."

1371

Should read "and mixture with"

This revision is now accepted and ready to land.Nov 8 2022, 7:56 AM
jeanPerier updated this revision to Diff 474031.Nov 8 2022, 9:29 AM
jeanPerier marked 15 inline comments as done.

Fix typos caught by Pete and remove paragaph about the "split"
of variables into two categories, that is more an implementation
detail about how one can view things and it's too much detail for
this document.

jeanPerier marked an inline comment as done.Nov 8 2022, 9:57 AM

Thanks @PeteSteinfeld for the detailed review !

flang/docs/HighLevelFIR.md
108–113

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.

1337–1339

Changed to Instead of restricting .... the hlfir.declare op is not very important here.

peixin accepted this revision.Nov 9 2022, 7:32 PM

One thing I am not clear is that when do you plan to use fir.declare given hlfir.declare is used. Why is the debug info not added to hlfir.declare directly?

flang/docs/HighLevelFIR.md
97
288
jeanPerier marked 3 inline comments as done.Nov 10 2022, 12:32 AM

One thing I am not clear is that when do you plan to use fir.declare given hlfir.declare is used. Why is the debug info not added to hlfir.declare directly?

Lowering from the parse tree will only generate hlfir.declare, then in the HLFIR to FIR conversion pass that gets rid of HLFIR operations, hlfir.declare will be translated to fir.declare that is slightly simpler.

hlfir.declare conveys the information that will allow generate variable debug info, and this role is then transmitted to fir.declare until LLVM IR dialect is generated and DIE debug info is actually generated (as I envision it, there is no detailed plan for debug info yet).

Fix more typos.

While for the near term, hlfir.declare will simplify the OpenMP privatisation clause handling but when we bring in the privatisation clauses to the OpenMP dialect, the dialect will have to live with multiple variable types (hlfir.declare, fir.declare, fir.alloca, llvm.alloca etc).

flang/docs/HighLevelFIR.md
96

Nit: passe -> passes/pass.

114

Will these two co-exist? Or will fir.declare be generated from hlfir.declare ?

125

fir.declare or hlfir.declare?

1337

Something is missing in this sentence.

Thanks for describing the changes in detail and for putting them up for review before the code changes. I have a few questions. None of them are blocking, so feel free to push forward if necessary.

It will be good if you can mention in the summary that this patch introduces hlfir.declare operation. It will be great if we can have a table/summary describing what information is present in hlfir.declare vs fir.declare.

flang/docs/HighLevelFIR.md
76

Is this not allowed for an hlfir variable or for fir variables too?

95

Would this information be available in fir.declare as well? Or should any pass requiring such information operate at the hlfir level?

283–284

Is it not possible to create %var#1 from %var#0?

One thing I am not clear is that when do you plan to use fir.declare given hlfir.declare is used. Why is the debug info not added to hlfir.declare directly?

Lowering from the parse tree will only generate hlfir.declare, then in the HLFIR to FIR conversion pass that gets rid of HLFIR operations, hlfir.declare will be translated to fir.declare that is slightly simpler.

hlfir.declare conveys the information that will allow generate variable debug info, and this role is then transmitted to fir.declare until LLVM IR dialect is generated and DIE debug info is actually generated (as I envision it, there is no detailed plan for debug info yet).

Got it. Thanks a lot.

jeanPerier marked 3 inline comments as done.Nov 10 2022, 4:54 AM

Thanks for the rewiew

While for the near term, hlfir.declare will simplify the OpenMP privatisation clause handling but when we bring in the privatisation clauses to the OpenMP dialect, the dialect will have to live with multiple variable types (hlfir.declare, fir.declare, fir.alloca, llvm.alloca etc).

They are three things that could maybe help here:

  1. hlfir.declare and fir.declare share a common FortranVariableOpInterface that could be relied on
  2. hlfir.declare and fir.declare will probably not co-exists (so depending on when privatization occur, only one or the other would matter)
  3. the declare ops are not really doing anything with memory, they are just tagging it, and binding info to it, so maybe it is OK to only care about the alloc ops as far as privatization is concerned.
flang/docs/HighLevelFIR.md
114

It is possible for the two to coexist, but the plan is for fir.declare to be generated from hlfir.declare while converting HLFIR to FIR.

125

hlfir.declare, thanks

1337

Repharsed to "Instead of restricting the memory types an HLFIR variable can have, it was"

jeanPerier marked 2 inline comments as done.

Fix two typos and add missing word.