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
136

move into the omp namespace and add doxygen documentation.

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

Also elsewhere.

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

Also similarly elsewhere.

2194
2264

Remove all the llvm:: in this file.

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

This code can be removed.

6328

bool

6329

hasClausesOfKind should be better

6334

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

6344–6345

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

6350–6351

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

6355–6358

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
6302–6357

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
6302–6357

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
6330–6331

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

6335–6336

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
6330–6331

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

6335–6336

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.