Page MenuHomePhabricator

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

Authored by Meinersbur on Jan 20 2022, 1:59 PM.

Diff Detail

Event Timeline

Meinersbur created this revision.Jan 20 2022, 1:59 PM
Meinersbur requested review of this revision.Jan 20 2022, 1:59 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 20 2022, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 3:38 PM
peixin edited the summary of this revision. (Show Details)Mar 31 2022, 6:11 PM
peixin added reviewers: shraiysh, NimishMishra.
peixin edited the summary of this revision. (Show Details)
peixin added subscribers: NimishMishra, shraiysh.

Added @shraiysh and @NimishMishra . They may be more familiar to this code.

LGTM. Can we please rebase this and fix the same in convertOmpAtomicUpdate and convertOmpAtomicCapture too?

shraiysh accepted this revision.Mar 31 2022, 9:39 PM
This revision is now accepted and ready to land.Mar 31 2022, 9:39 PM
shraiysh added inline comments.Mar 31 2022, 9:43 PM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
79–80

[Suggestion] We probably don't need to do this, because if a conversion does not require an alloca (for example, barrier), we will be creating a basicblock unnecessarily. So, creating this on-demand seems okay to me.

Meinersbur marked an inline comment as done.Apr 4 2022, 8:18 PM
Meinersbur added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
79–80

Making decision based on the current position of an IRBuilder creates heaps of problems and unpredictability. Special cases like this one here need to be tested and maintained, and still easily introduce bugs. An unconditional dedicated alloca block avoids all these problems.

An extra basic block on the other side is insignificant. It will be gone as soon as simplify-cfg runs.

However, this seems to be controversial, so I removed the TODO for now.

Meinersbur updated this revision to Diff 420371.Apr 4 2022, 8:21 PM
Meinersbur marked an inline comment as done.
  • Rebase
  • Fix convertOmpAtomicUpdate, convertOmpAtomicCapture, and convertOmpOrdered
This revision was landed with ongoing or failed builds.Apr 5 2022, 10:37 AM
This revision was automatically updated to reflect the committed changes.