This is an archive of the discontinued LLVM Phabricator instance.

[OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createParallel.
ClosedPublic

Authored by Meinersbur on Jan 13 2022, 7:22 AM.

Details

Summary

When a Builder methods accepts multiple InsertPoints, when both point to
the same position, inserting instructions at one position will "move" the
other after the inserted position since the InsertPoint is pegged to the
instruction following the intended InsertPoint. For instance, when
creating a parallel region at Loc and passing the same position as AllocaIP,
creating instructions at Loc will "move" the AllocIP behind the Loc
position.

To avoid this ambiguity, add an assertion checking this condition and
fix the unittests.

In case of AllocaIP, an alternative solution could be to implicitly
split BasicBlock at InsertPoint, using the first as AllocaIP, the second
for inserting the instructions themselves. However, this solution is
specific to AllocaIP since AllocaIP will always have to be first. Hence,
this is an argument to generally handling ambiguous InsertPoints as API
sage error.

Diff Detail

Event Timeline

Meinersbur created this revision.Jan 13 2022, 7:22 AM
Meinersbur requested review of this revision.Jan 13 2022, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 7:22 AM
jdoerfert accepted this revision.Jan 13 2022, 7:37 AM

LG, thanks!

This revision is now accepted and ready to land.Jan 13 2022, 7:37 AM

Please fix the CI fail for openmp-llvm.mlir.

Nit: It seems the same code is codded to quite a few tests. Would it make sense to factor out the common portion?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
542

Nit: me -> be

  • fix check-mlir
Herald added a project: Restricted Project. · View Herald Transcript

Nit: It seems the same code is codded to quite a few tests. Would it make sense to factor out the common portion?

A subsequent patch will introduce functionality to make the splitting of BB as the current IP easier, most likely reducing it to a single line.

If we want to avoid repetitive code, the more complete solution would be to factor out the entire using InsertPointTy = ... prologue; for instance, in a fixture.

This revision was landed with ongoing or failed builds.Jan 20 2022, 8:14 AM
This revision was automatically updated to reflect the committed changes.
Meinersbur marked an inline comment as done.