This is an archive of the discontinued LLVM Phabricator instance.

[flang] Fix processing ModuleLikeUnit evaluationList
ClosedPublic

Authored by peixin on Feb 24 2022, 12:28 AM.

Details

Summary

Push the ModuleLikeUnit evalutionList when entering module unit. Pop it
when exiting module unit if there is no module procedure. Otherwise, pop
it when entering the first module procedure.

Diff Detail

Event Timeline

peixin created this revision.Feb 24 2022, 12:28 AM
peixin requested review of this revision.Feb 24 2022, 12:28 AM
Herald added a project: Restricted Project. · View Herald Transcript

https://github.com/flang-compiler/f18-llvm-project/pull/1184 is upstreamed in D120336. I noticed the function code in https://github.com/flang-compiler/f18-llvm-project/pull/1184 is not fully tested and there is one bug. This and D120459 address those two problems.

Can you check why the patch application fails?

flang/lib/Lower/PFTBuilder.cpp
321

What happens without this code?

Can you check why the patch application fails?

This is based on D120459. I had thought the whole file will be applied when applying the patch in Phabricator. I need to rebase this after D120459. Maybe I can merge them if it is easy for review?

flang/lib/Lower/PFTBuilder.cpp
321

This function enterConstructOrDirective is called by main program unit, module procedure, internal procedure, external procedure. Only to pop the evalutionlist when entering the module procedure unit since the evalutionlist stack must be empty for OpenMP/OpenACC declarative directives when calling enterFunction. Check https://github.com/llvm/llvm-project/blob/f3480390be614604907be447afa3f5ab10b97d04/flang/lib/Lower/PFTBuilder.cpp#L245-L286.

To be more clear, there are two cases need to be noticed:

$ cat file1.f90
module m
  integer x
  !$omp threadprivate(x)
end module
$ cat file2.f90
module m
  integer x
  !$omp threadprivate(x)
contains
  subroutine sub()
  end
end module

For file1.f90, just need to process the evalutionlist as if it is used in main program unit or internal procedure and the evalutionlist is popped when ending the module. For file2.f90, the evalutionlist cannot be popped when ending the module. Instead, need to pop it before entering the subroutine sub.

Can you check why the patch application fails?

This is based on D120459. I had thought the whole file will be applied when applying the patch in Phabricator. I need to rebase this after D120459. Maybe I can merge them if it is easy for review?

Added edge to that revision. This should help in patch application. For future ref, we can add a "Depends on DXXXXXX" line to the summary to automatically create this edge as mentioned here.

Added edge to that revision. This should help in patch application. For future ref, we can add a "Depends on DXXXXXX" line to the summary to automatically create this edge as mentioned here.

Got it. Thanks. Now CI passed.

peixin updated this revision to Diff 414292.Mar 10 2022, 1:06 AM
peixin retitled this revision from [flang] Fix lowering OpenMP/OpenACC declarative constructs in non-module unit to [flang] Fix processing ModuleLikeUnit evaluationList.
peixin edited the summary of this revision. (Show Details)

The previous design for lowering OpenMP/OpenACC declarative constructs in module is dirty. Refactor the code as follows:

EnterModule (push ModuleLikeUnit evalutionList)

[Enter OpenMP/OpenACC decl constructs] (push declconstruct ... as other directives)
[Exit OpenMP/OpenACC decl constructs] (pop declconstruct ... as other directives)
[Enter module procedure] (pop ModuleLikeUnit evalutionList)
[Exit module procedure]

ExitModule (pop ModuleLikeUnit evalutionList if there is no module procedure)

Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 1:06 AM
vdonaldson accepted this revision.Mar 10 2022, 10:15 AM

Looks ok to me.

This revision is now accepted and ready to land.Mar 10 2022, 10:15 AM
peixin updated this revision to Diff 414581.Mar 10 2022, 10:39 PM

Rebase to test CI before land.

This revision was automatically updated to reflect the committed changes.

Can you sync this change with fir-dev?

@clementval Sure. Will do it now.