This is an archive of the discontinued LLVM Phabricator instance.

[OMPIRBuilder] Add support for simdlen clause
ClosedPublic

Authored by psoni2628 on Jul 5 2022, 9:50 AM.

Details

Summary

This patch adds OMPIRBuilder support for the simdlen clause for the
simd directive. It uses the simdlen support in OpenMPIRBuilder when
it is enabled in Clang. Simdlen is lowered by OpenMPIRBuilder by
generating the loop.vectorize.width metadata.

Diff Detail

Event Timeline

psoni2628 created this revision.Jul 5 2022, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 9:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
psoni2628 requested review of this revision.Jul 5 2022, 9:50 AM
Herald added a project: Restricted Project. · View Herald Transcript
psoni2628 changed the repository for this revision from rC Clang to rG LLVM Github Monorepo.Jul 5 2022, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 9:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
psoni2628 removed a project: Restricted Project.Jul 5 2022, 9:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 9:54 AM
psoni2628 removed a project: Restricted Project.Jul 5 2022, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 9:59 AM
arnamoy10 added inline comments.Jul 5 2022, 12:28 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

I am just wondering whether we should have a check to make sure that we are processing the clauses of only simd directive here. Because the function takes a general OMPExecutableDirective as argument

psoni2628 added inline comments.Jul 5 2022, 1:08 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

That's a fair point. I guess isSupportedByOpenMPIRBuilder could be used for other directive types other than simd, even though it's not right now.

psoni2628 marked an inline comment as not done.Jul 5 2022, 1:16 PM
psoni2628 added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

Would it make more sense to only guard the checking of clauses with a check for OMPSimdDirective, or the whole thing? I believe even the code below, which checks for an ordered directive, is also specifically for simd?

Example of guarding the whole thing:

if(dyn_cast<OMPSimdDirective>(S)) {
  // Check for unsupported clauses
  for (OMPClause *C : S.clauses()) {
    // Currently only simdlen clause is supported
    if (dyn_cast<OMPSimdlenClause>(C))
      continue;
    else
      return false;
  }

  // Check if we have a statement with the ordered directive.
  // Visit the statement hierarchy to find a compound statement
  // with a ordered directive in it.
  if (const auto *CanonLoop = dyn_cast<OMPCanonicalLoop>(S.getRawStmt())) {
    if (const Stmt *SyntacticalLoop = CanonLoop->getLoopStmt()) {
      for (const Stmt *SubStmt : SyntacticalLoop->children()) {
        if (!SubStmt)
          continue;
        if (const CompoundStmt *CS = dyn_cast<CompoundStmt>(SubStmt)) {
          for (const Stmt *CSSubStmt : CS->children()) {
            if (!CSSubStmt)
              continue;
            if (isa<OMPOrderedDirective>(CSSubStmt)) {
              return false;
            }
          }
        }
      }
    }
  }
}
domada added a subscriber: domada.Jul 5 2022, 11:35 PM
domada added inline comments.Jul 6 2022, 12:16 AM
clang/test/OpenMP/irbuilder_simd.cpp
15

Could you add separate test case instead of modifying existing test case?

shraiysh added inline comments.Jul 6 2022, 1:21 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

Can we instead have separate isSupportedByOpenMPIRBuilder for every directive to avoid bloating the function with checks and if conditions?

static bool isSupportedByOpenMPIRBuilder(const OMPSimdDirective &S) {...}
void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {...}

static bool isSupportedByOpenMPIRBuilder(const OMPOrderedDirective &S) {...}
void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective &S) {...}

static bool isSupportedByOpenMPIRBuilder(const OMPTaskDirective &S) {...}
void CodeGenFunction::EmitOMPOrderedDirective(const OMPTaskDirective &S) {...}
clang/test/OpenMP/irbuilder_simd.cpp
74

nit: maybe add newline

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1773

Can we please add this as a new test instead of modifying the existing one?

psoni2628 added inline comments.Jul 6 2022, 5:26 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

It was decided in D114379 to use OMPExecutableDirective in order to allow this function to be reused for constructs such as for simd. Do you wish to undo this now, and instead specialize the function?

shraiysh added inline comments.Jul 6 2022, 6:37 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

I wasn't aware of the earlier discussion about this. However it seems like it was suggested to try either splitting the function or merging it with slight preference for merging (I didn't understand how that would help constructs like for simd because I haven't worked with for simd much and will take others' word for it).

The suggested code change in previous reply - checking of clauses with a check for OMPSimdDirective - this seems like it would eventually become a huge function because of such checks.

@Meinersbur it would be great if you could please advise on this - if keeping the checks for all executable directives in the same function will be helpful in the long run - allowing us to reuse checks or should we split it? A third alternative would be to have both functions, one specialized for SIMD and one for ExecutableConstruct, where the latter might handle combined constructs like for simd etc. Would that arrangement work?

psoni2628 updated this revision to Diff 442555.Jul 6 2022, 6:59 AM
psoni2628 marked 3 inline comments as done.
  • Create new tests instead of modifying existing ones
  • Specialize isSupportedByOpenMPIRBuilder for OMPSimdDirective
clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

I agree with you that having a general function for OMPExecutableDirective will probably result in a very large, bloated function. I have tentatively just specialized the function for OMPSimdDirective.

Nit: Also add to the summary that this patch uses the simdlen support in OpenMPIRBuilder when it is enabled in Clang.

clang/lib/CodeGen/CGStmtOpenMP.cpp
2600

Nit: Else after return/continue is discouraged.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

psoni2628 added a comment.EditedJul 6 2022, 8:04 AM

Nit: Also add to the summary that this patch uses the simdlen support in OpenMPIRBuilder when it is enabled in Clang.

I just wanted to clarify that below is what you mean?

This patch adds OMPIRBuilder support for the simdlen clause for the
simd directive. It uses the simdlen support in OpenMPIRBuilder when
it is enabled in Clang.

Nit: Also add to the summary that this patch uses the simdlen support in OpenMPIRBuilder when it is enabled in Clang.

I just wanted to clarify that below is what you mean?

This patch adds OMPIRBuilder support for the simdlen clause for the
simd directive. It uses the simdlen support in OpenMPIRBuilder when
it is enabled in clang.

Yes. Thanks.

Maybe you can also add that simdlen is lowered by the OpenMPIRBuilder by generating the loop.vectorize.width metadata.

psoni2628 updated this revision to Diff 442595.Jul 6 2022, 8:21 AM
psoni2628 edited the summary of this revision. (Show Details)
psoni2628 edited the summary of this revision. (Show Details)
  • Remove discouraged else after return
psoni2628 marked an inline comment as done.Jul 6 2022, 8:23 AM

Generally, this is fine. Some nits. The only reason not to accept this right now is the test. Why manual check lines?
Wrt. the function signature. As soon as we have more then SIMD directives to check we refactor things. Keep it simple
until you need the functionality.

clang/lib/CodeGen/CGStmtOpenMP.cpp
2598
clang/test/OpenMP/irbuilder_simdlen.cpp
2

The check lines look auto-generated and then modified by hand. Why is that?

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

No debug location needed. You also copied the comment that makes little sene.

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

Also above, no llvm::

psoni2628 added inline comments.Jul 6 2022, 8:49 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
2923

It isn't used in the original applySimd either. Should I remove it in both places?

psoni2628 marked an inline comment as not done.Jul 6 2022, 8:49 AM
psoni2628 added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
608

It isn't used in the original applySimd either. Should I remove it in both places?

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

Wrong comment to reply to. Please ignore this.

jdoerfert added inline comments.Jul 6 2022, 9:07 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
608

Yes.

psoni2628 added inline comments.Jul 6 2022, 10:26 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
608

Looks like I'll need to fix applySimd on the MLIR side too. I think it makes more sense to do that separately in a different patch. For now, I will leave applySimd with the unused DebugLocation. I will fix applySimdlen.

psoni2628 updated this revision to Diff 442637.Jul 6 2022, 11:04 AM
  • Autogenerate check lines for test case
  • Use isa instead of dyncast
  • Remove unused DebugLoc
  • Remove llvm:: from llvm::ConstantInt
psoni2628 marked 8 inline comments as done.Jul 6 2022, 11:06 AM
psoni2628 added inline comments.
clang/test/OpenMP/irbuilder_simdlen.cpp
2

I originally took irbuilder_simd.cpp and modified it to include simdlen. I have now auto-generated the check lines.

psoni2628 marked an inline comment as done.Jul 6 2022, 11:08 AM
jdoerfert accepted this revision.Jul 6 2022, 12:00 PM

LG, let's update the test script first though. See below.

clang/test/OpenMP/irbuilder_simdlen.cpp
2

I see. Auto generated is great, but you don't actually check the metadata anymore. Add --check-globals to the update script command.
You might also want to teach the script about access group metadata, see D101742 for an example how to do this.

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

As mentioned before. No llvm::. This is not MLIR.

This revision is now accepted and ready to land.Jul 6 2022, 12:00 PM

Why not have simdlen an argument to applySimd (which can be null if not used)? In this case it happens to be just adding some metadata, but for others such as unrollLoopPartial it does not make sense to have the unroll factor set by a different method. It would also allow better consistency checks if passed as a parameter.

clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

A function taking only OMPSimdDirective will still not handle OMPForSimdDirective and the code for handling it would need to be duplicated for both (and OMPDistributeSimdDirective, and OMPDistributeParallelForSimdDirective, ...)

I'd expect the check the most common to be the (non-)existence of a clause such as simdlen. Once support for it is added, it should be supported for all combined/composite constructs. Code Duplication not necessary. We do not need to check whether OMPExecutableDirective is is a simd construct because Sema will reject simdlen on any other construct anyway.

Therefore is still favor taking a OMPExecutableDirective argument. OpenMPIRBuilder does not support combined/composite construct yet, but whoever adds it has to face the immense problem of refactoring all the isSupportedByOpenMPIRBuilder implementations to have to rewrite that function for all combined/composite constructs. Better consider the possibility of combined/composite constructs from the beginning.

Why not have simdlen an argument to applySimd (which can be null if not used)? In this case it happens to be just adding some metadata, but for others such as unrollLoopPartial it does not make sense to have the unroll factor set by a different method. It would also allow better consistency checks if passed as a parameter.

Sure.

clang/lib/CodeGen/CGStmtOpenMP.cpp
2596–2599

Unsure why this has to go into another round of comments:

This is, as I noted, a mood discussion. If this passes all tests there is no good reason to make it OMPExecutableDirective right now. As we add anything that does not work with this API we need to make a choice and include changes anyway. Arguing the type of the argument right now, = the ~15 letter difference, is making anything harder in the future, is arguably not true. As always, designing something without coverage and use case at hand is discouraged, especially if the design does not exclude an extension.

psoni2628 updated this revision to Diff 442715.Jul 6 2022, 4:33 PM
psoni2628 marked 4 inline comments as done.
  • Remove applySimdlen and apply simdlen in applySimd
  • Remove unnecessary DebugLocation from applySimd in both OpenMPIRBuilder and Flang MLIR
  • Add llvm.access.group to llvm/utils/UpdateTestChecks/common.py
  • Fix test case by adding --check-globals
Herald added a project: Restricted Project. · View Herald Transcript
psoni2628 updated this revision to Diff 443622.Jul 11 2022, 6:35 AM
  • Rebase, ensuring that build bot failures are gone
This revision was landed with ongoing or failed builds.Jul 11 2022, 10:25 AM
This revision was automatically updated to reflect the committed changes.