This is an archive of the discontinued LLVM Phabricator instance.

OMPIRBuilder for Interop directive
ClosedPublic

Authored by sriharikrishna on Jul 13 2021, 1:18 AM.

Details

Summary

Implements the OMPIRBuilder portion for the
Interop directive.

Diff Detail

Event Timeline

sriharikrishna created this revision.Jul 13 2021, 1:18 AM
sriharikrishna requested review of this revision.Jul 13 2021, 1:18 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 13 2021, 1:18 AM

There are 3 clang tidy warnings to address. Overall this is a good first step. @ABataev @mikerice is there any major problem with this? If not we should try to get it in asap and improve it in tree.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
121

move into the omp namespace and add doxygen documentation.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1021

Also elsewhere.

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

Also similarly elsewhere.

2887
2957

Remove all the llvm:: in this file.

ABataev added inline comments.Jul 13 2021, 2:01 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6578–6579

This code can be removed.

6596

bool

6597

hasClausesOfKind should be better

6602

EmitLValue(C->getInteropVar()).getPointer(*this);.

6612–6613

EmitLValue(C->getInteropVar()).getPointer(*this);.

6618–6619

EmitLValue(C->getInteropVar()).getPointer(*this);.

6623–6626

Is this allowed at all? If not, better to turn it into an assert.

clang/test/OpenMP/interop_irbuilder.cpp
2

Actual codegen checks are missing

Address reviewer comments

ABataev added inline comments.Jul 14 2021, 6:51 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6570–6625

Could you merge these asserts and put them as preconditions for the if-else chain somehow?

clang/test/OpenMP/interop_irbuilder.cpp
2

Still no checks, need to add | FileCheck %s to the RUN line

OMPIRBuilder for Interop directive

you need to squash your commits locally and update this revision with a single commit/diff.

clang/lib/CodeGen/CGStmtOpenMP.cpp
6570–6625

This is not how assertions work.

assert(EXPR && "MESSAGE");

OMPIRBuilder for Interop directive. Squashed commit

OMPIRBuilder for Interop directive. Squashed commit

ABataev added inline comments.Jul 23 2021, 7:35 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6598–6599

This code is common for all if-else braches, move out of the conditional blocks?

6603–6604

Can we have anything else rather than C->getIsTargetSync() here? If no, then it should look like this:

if (C->getIsTarget()) {
  InteropType = llvm::omp::OMPInteropType::Target;
} else {
  assert(C->getIsTargetSync() && "Expected ...");
  InteropType = llvm::omp::OMPInteropType::TargetSync;
}
sriharikrishna marked an inline comment as done.Jul 26 2021, 11:02 PM
sriharikrishna added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
6598–6599

Moving the common line out will require checking the kind of clause in the directive. So, I prefer to leave it as is.

6603–6604

The standard only allows for Target and TargetSync.

sriharikrishna marked an inline comment as done.

OMPIRBuilder for Interop directive

OMPIRBuilder for Interop directive

jdoerfert accepted this revision.Sep 9 2021, 1:01 PM

LGTM,

please rebase on top of trunk and use ConstantInt::get(Int32, 0) instead of the APInt way.

This revision is now accepted and ready to land.Sep 9 2021, 1:01 PM

OMPIRBuilder for Interop directive

sriharikrishna marked 2 inline comments as done.Sep 13 2021, 12:58 PM
This revision was landed with ongoing or failed builds.Jan 27 2022, 11:53 AM
This revision was automatically updated to reflect the committed changes.