Page MenuHomePhabricator

OMPIRBuilder for Interop directive
AcceptedPublic

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
136

move into the omp namespace and add doxygen documentation.

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

Also elsewhere.

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

Also similarly elsewhere.

2757
2827

Remove all the llvm:: in this file.

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

This code can be removed.

6462

bool

6463

hasClausesOfKind should be better

6468

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

6478–6479

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

6484–6485

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

6489–6492

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
6436–6491

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
6436–6491

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
6464–6465

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

6469–6470

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
6464–6465

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

6469–6470

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.Thu, Sep 9, 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.Thu, Sep 9, 1:01 PM

OMPIRBuilder for Interop directive

sriharikrishna marked 2 inline comments as done.Mon, Sep 13, 12:58 PM