Page MenuHomePhabricator

[Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

Authored by fghanim on May 9 2020, 12:44 PM.



Modified the OMPBuilderCBHelpers in the following ways:

  • Moved location of class definition and deleted all constructors
  • Moved OpenMP-specific address allocation of local variables
  • Moved threadprivate variable creation for the current thread

Diff Detail

Event Timeline

fghanim created this revision.May 9 2020, 12:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2020, 12:44 PM
jdoerfert added inline comments.May 12 2020, 5:50 PM

Is this needed?


Nit: Make it a doxygen comment with 3x`/, -will` + full stop.


Nit: No need.


What is wrong with BB->getFirstNonPHIOrDbgOrLifetime()? If it is to avoid too many test changes, please add a TODO. I think from a functional perspective there is no need to advance it after the first set of allocas that may be separated from the phis by non-allocas?

fghanim marked 4 inline comments as done.May 14 2020, 9:22 AM
fghanim added inline comments.

All these are changes that I forgot to remove after I created a special version for EmitOMPFirstprivateClause in D79677 for the OMPBuilder


Nothing wrong with it. The first while loop, I just came across a case that I don't remember at the moment that had an instruction that wasn't an alloca, and I wanted to skip over that.
the second while, is to get to the first non-alloca instruction. The purpose is to be able to insert new allocas, after already generated allocas.

fghanim updated this revision to Diff 264012.May 14 2020, 9:23 AM
fghanim marked an inline comment as done.

updating in response to review comments

jdoerfert accepted this revision.May 14 2020, 9:08 PM

Interesting that no tests changed but I guess we just start the move to the IRBUilder.

I think there are two minor comments from before that need to be addressed and the alloca thing, otherwise LGTM.


There is no need to do that. It is fine to insert allocas anywhere in the block.

This revision is now accepted and ready to land.May 14 2020, 9:08 PM
fghanim updated this revision to Diff 269584.Jun 9 2020, 10:01 AM
  • rebase
  • addressing reviewer's comments
This revision was automatically updated to reflect the committed changes.