Page MenuHomePhabricator

[OpenMPIRBuilder] Implement collapseLoops.

Authored by Meinersbur on Dec 14 2020, 7:41 PM.



The collapseLoops method implements a transformations facilitating the implementation of the collapse-clause. It takes a list of loops from a loop nest and reduces it to a single loop that can be used by other methods that are implemented on just a single loop, such as createStaticWorkshareLoop.

This patch shares some changes with D92974 (such as adding some getters to CanonicalLoopNest), used by both patches.

Diff Detail

Event Timeline

Meinersbur created this revision.Dec 14 2020, 7:41 PM
Meinersbur requested review of this revision.Dec 14 2020, 7:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 7:41 PM
Herald added a subscriber: sstefan1. · View Herald Transcript

Cool, really happy to see this. I tried to follow the logic but had some questions. Most comments are nits though.


Nit: Assert message.


Nit: auto *Pred at least.


Nit: Maybe do ... while(Changed) but that's just an idea.


Do we really need/want to do such a search here? We could leave them be I guess, or delete them as we make them dead?

Interesting choices for sizes of the set and vector, I'm curious why


Nit: Can we add a TODO saying that an OpenMP sanitizer could use overflow tracking multiplication here and report an error if it does overflow.


How can ContinueBlock in 1297 be non-null? Am I reading this wrong or are we using ContinueBlock once and then ContinuePred for all other loops?


Maybe we want to move this body into a helper, it appears twice, unclear if that helps.


Again I don't see how ContinueBlock is non-null, actually, even 1309 it is always null, isn't it? Something seems to be off, maybe the two are swapped?

Meinersbur marked 5 inline comments as done.
  • Rebase after commit of D92974
  • Address review comments
jdoerfert accepted this revision.Feb 3 2021, 8:13 AM

It's a little hard to determine what this does from the code and unit test. It looks reasonable. I'm fine with including it for now as it won't harm.


As far as I can tell, IncludeNextSrcBB is always false.

This revision is now accepted and ready to land.Feb 3 2021, 8:13 AM
Meinersbur updated this revision to Diff 321262.Feb 3 2021, 4:46 PM
  • Address @jdoerfert's remark
  • Avoid 'comparison of integer expressions of different signedness' warning
This revision was landed with ongoing or failed builds.Feb 3 2021, 5:12 PM
This revision was automatically updated to reflect the committed changes.