This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][MLIR] Fix for nested parallel regions
ClosedPublic

Authored by kiranchandramohan on Oct 2 2020, 12:50 AM.

Details

Summary

[MLIR][OpenMP] Fix for nested parallel regions

Fix contains the following changes,

  1. Don't set the insertion point in the body callback.
  2. Save the continuation IP in a stack and set the branch to continuation IP at the terminator.

Note: This is required for supporting the master Operation (https://reviews.llvm.org/D87247).

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
kiranchandramohan requested review of this revision.Oct 2 2020, 12:50 AM

@SouraVX In this patch I have removed the OpenMPIRBuilder alloca changes. This contains only the changes to in ModuleTranslation and it seems to work fine. Can you check at your end as well as with the master Operation?

@SouraVX In this patch I have removed the OpenMPIRBuilder alloca changes. This contains only the changes to in ModuleTranslation and it seems to work fine. Can you check at your end as well as with the master Operation?

Thanks! for the patch @kiranchandramohan :). I tried/applied this patch locally, it is working as great/expected(i.e MasterOp is getting lowered correctly(verifed the well formedness of the IR by generating objects)).

However I haven't reviewed the code/patch fully(since I was OOO). Will look in as soon as I can.

ftynse accepted this revision.Oct 5 2020, 1:36 AM
ftynse added inline comments.
mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
133

Nit: SmallVector is re-exported, no need to prefix it with llvm namespace.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
505

I'd much rather have IRBuilder used here for consistency with the rest of the code, even if it would require setting the insertion point twice.

This revision is now accepted and ready to land.Oct 5 2020, 1:36 AM
SouraVX added inline comments.Oct 7 2020, 12:16 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
460

Is this TODO still valid ? @jdoerfert .
Patch fixes the problem in hand, i.e it's the reset of CodeGenIP that was RC for this.

Using the builder to create branch instruction for a terminator.
Added one more assertion.
Added on more test.

kiranchandramohan marked an inline comment as done.Oct 7 2020, 2:37 AM

@SouraVX do you have more comments?

Fixed the comment to remove the llvm prefix for smallvector.

@SouraVX I believe some changes are required for handling the allocaIP, but as you say it is not related to the nested parallel region issue.

kiranchandramohan marked an inline comment as done.Oct 7 2020, 3:02 AM
SouraVX accepted this revision.Oct 7 2020, 8:02 AM

Thanks for the patch. You may want to wait for @jdoerfert for final sign off!

ping @jdoerfert. Is this patch OK? This fixes the nested parallel regions issue and will unblock the master op lowering (https://reviews.llvm.org/D87247).

This revision was automatically updated to reflect the committed changes.