[OMPIRBuilder] Add support for simd (loop) directive.

[arnamoy10]

Summary

This patch adds OMPIRBuilder support for the simd directive (without any clause). This will be a first step towards lowering simd directive in LLVM_Flang. The patch uses existing CanonicalLoop infrastructure of IRBuilder to add the support. Also adds necessary code to add llvm.access.group and llvm.loop metadata wherever needed.

[tschuett]

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

520

I believe LLVM uses American English.

[tschuett]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

2121

Properties is unused in the function?

[Meinersbur]

Thanks for helping to complete the OpenMPIRBuilder implementation!

clang/lib/CodeGen/CGStmtOpenMP.cpp

2586–2587

[nit] Unnecessary whitespace

2587

Did you consider adding a check whether OpenMPIRBuilder actually implements all features requested before using it? See isSupportedByOpenMPIRBuilder.

2594

[style] According to the LLVM coding standard, auto is only used in specific cases.

clang/test/OpenMP/simd_codegen_irbuilder.cpp

16

Could you use regexes to match the virtual register names? The script update_cc_checks.py may help with this, see the other tests.

The naming scheme for files testing the OpenMPIRBiulder is usually irbuilder_<constructs>

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

524

Capitalizing SIMD leads to long runs of capital letters. While not an LLVM coding style rule, it is still common to title-case acronyms in identifiers if the acronym is ~4 letters or more, such as OMPSimdDirective instead of OMPSIMDDirective.

For methods transformation CanonicalLoopInfo, we don't use createXYZ naming scheme which is reserved for inserting something new. Suggestions: applySimd, vectorizeLoop, or simdizeLoop.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

2120

The LLVM coding style still capitalizes parameters and variables.

2123

The property also applies to any memory access, e.g. memset. You could use mayWriteToMemory() and mayReadFromMemory().

2126

[serious] The access group must be unique only to the loop being accessed.

2157–2168

[serious] header and cond don't contain any memory accesses. body may consist of multiple basic blocks, of which this only applies to the first one.

[arnamoy10]

clang/lib/CodeGen/CGStmtOpenMP.cpp

2586–2587

I took a look at isSupportedByOpenMPIRBuilder() and seems like it is more suitable for a check against work-sharing loop construct, not for simd.

And for simd, I am not sure what features I should be checking for. One check I can think of is checking whether any of the not-yet-supported clauses is present in the simd directive or not. Am I on the right track?

2594

I am inspired by existing code though

[Meinersbur]

clang/lib/CodeGen/CGStmtOpenMP.cpp

2586–2587

Indeed, isSupportedByOpenMPIRBuilder is for the workshoring-loop, you could introduce another one for simd. Or modify it to accept OMPExecutableDirective as an argument. This might be useful for combined constructs such as for simd.

Best approach IMHO is to whitelist clauses. Reject any clause you did not test/add support yet. This way we can fallback to the non-OpenMPIRBuilder codegen and not break things that worked without -fopenmp-enable-irbuilder (a lot of code already does not work -fopenmp-enable-irbuilder, so don't put too much effort into getting the interaction between OpenMPIRBuilder and current codegen to work; but we need an overview what is already supported and which clauses are still require some work)

2594

There already is a lot of code violating the coding standard, either because the coding standard changed or it slipped through a review. A prominent example is that the methods of the IRBuilder are still PascalCase, although the current standard says they should use camelCase. Same applies to SourceLocToDebugLoc itself (which every time I see it makes me think SourceLocToDebugLoc is a type)

Not a reason to not use the newest coding standard for new code.

[arnamoy10]

Addressing reviewers comments Major changes are as follows:

1. Skipping unsupported clauses and skip the case when there is an ordered directive inside the simd construct
2. Traversing all the blocks in the Canonical loop body (not only the first block of the loop body) to look for locations to insert metadata
3. Making metadata unique for each Canonical loop
4. Update test case to reflect changes.

[Meinersbur]

clang/lib/CodeGen/CGStmtOpenMP.cpp

2588–2594

I think it would be better to list the clauses that are supported. In addition to being shorter, it would also reject any clause we forgot about and new ones.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

2122

[style] According to the LLVM coding standard, auto is only used in specific cases.

2124

[style] Please use the current LLVM coding style.

2126

[serious] This does not create a LoopID. A LoopID is a bag of properties where the first element points to itself (which automatically makes then distinct). E.g.

!0 = distinct !{!0, !{!"llvm.loop.parallel_accesses", !1}}

where !1 is an access group (eg !1 = distinct !{}).

[serious] The instruction's metadata should not be a LoopID, but a access group (eg !1 for the above example). I think you should not pass an array of properties, but just the access group MDNode.

2151

[serious] The llvm.loop.parallel_accesses property takes an argument: the access group.

2156–2157

[Style] Please use the current LLVM coding standard.

2181

[serious] Using DominatorTree to check whether we passes Exit is redundant: For any block that is dominated by Exit (by definition of dominator) we will have to visited Exit first anyway. Therefore not adding Exit itself would be sufficient.

You actually don't want paths that go to exit, but the basic blocks that come from Body (the successor of Cond) to the Continue/Latch block. By the definition of CanonicalLoopInfo, any block reachable from Body can only go to Continue[1], so it would be sufficient to iterate everything reachable from Body, including Body itself, until we hit the Latch. Header and Cond themselves are not allowed to contain any memory accesses.

The safest way to determine the set of blocks inside the loop would be using LoopInfo, which internally uses DominatorTree, but for checking dominance with the header, not the exit.

[1] There are exceptions: paths can end in an unreachable instruction and I am currently working on defining what happens at cancellation[2]. There will also be a BasicBlock like Latch/Continue that signals the end of the loop. In both cases these still contain blocks that are not inside the loop in LoopInfo's sense. However, marking instructions in them with access groups should is not a problem since the access groups marker only has an effect if inside a loop that has the llvm.loop.parallel_accesses property set to that access group.

[2] I found interesting cases in which Clang re-uses the same basic blocks (cleanup block for destructors and exception handling) for multiple paths by using a PHINode and a switch to branch to the scope it came from. That is, if part of a loop it would always branch back to another block in the loop, but statically any other switch target would be reachable as well. Not sure yet whether this will be a problem that requires us to use LoopInfo.

[arnamoy10]

Thanks @Meinersbur for the comments. Addressing reviewers comments:

1. Update code to make the matadata addition correct, as per reviewers comments.
2. Using LoopInfo to identify BasicBlocks to iterate through.
3. Updating the test case.
4. Following coding standard.

[Meinersbur]

[testing] Could you add a test for applySimd into OpenMPIRBuilderTests.cpp, so we have a test that is independent of Clang?

clang/test/OpenMP/irbuilder_simd.cpp

3 ↗

(On Diff #394046)

The invocation is identical to the RUN on line 1. CHECKTWOLOOPS is not needed and can also be made as CHECK.

4 ↗

(On Diff #394046)

A single expected-no-diagnostics is sufficient

67–69 ↗

(On Diff #394046)

If you added these CHECK lines by hand, remove those that are not relevant

71 ↗

(On Diff #394046)

Do not add a regex specification for any but the first occurance. A regex specification will "overwrite" the previous match, i.e. the different occurrences of META4 would not need to be the same.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

2119

2123

2124

The instruction may already have an access group assigned e.g. from a nested #pragma clang loop vectorize(assume_safety). This would overwrite the access group. See LoopInfoStack::InsertHelper (CGLoopInfo.cpp) on how to assign multiple access groups, or add a TODO.

2158

Compiler warning: warning C4189: 'DT': local variable is initialized but not referenced

2164

OpenMPIRBuilder is part of the llvm namespace, llvm:: is not necessary.

2168

Could you add a todo note such as:

// TODO: Generalize getting all blocks inside a CanonicalizeLoopInfo, preferably without running any passes.

2175–2177

[style]

2183

The loop might already have a llvm.loop.parallel_accesses, in which case those two lists could be combined.

I don't think its very relevant, but maybe add a TODO?

[Meinersbur]

clang/lib/CodeGen/CGStmtOpenMP.cpp

2587

2591

[arnamoy10]

Updating as per reviewers comments.

Adding a test case in OpenMPIRBuilderTest.cpp

[Meinersbur]

LGTM. Thanks for the patch.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1696 ↗

(On Diff #400631)

It would also be possible to use any_of instead of a loop.

[arnamoy10]

1. Trying to tackle a test failure
2. Adding any_of() as per reviewer's suggestion.

[Meinersbur]

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

1691–1696 ↗

(On Diff #400830)

Can make it a one-liner.

[arnamoy10]

Making any_of one-liner.

clang/lib/CodeGen/CGStmtOpenMP.cpp

static void emitOMPSimdRegion(CodeGenFunction &CGF, const OMPLoopDirective &S,
   // Emit: if (PreCond) - end.
   if (ContBlock) {
     CGF.EmitBranch(ContBlock);
     CGF.EmitBlock(ContBlock, true);
   }
 }
 
 void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {
+
+  bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+  if (UseOMPIRBuilder) {
+    auto &&CodeGenIRBuilder = [this, &S, UseOMPIRBuilder](CodeGenFunction &CGF,
+                                                          PrePostActionTy &) {
+      // Use the OpenMPIRBuilder if enabled.
+      if (UseOMPIRBuilder) {
+        // Emit the associated statement and get its loop representation.
+        auto DL = SourceLocToDebugLoc(S.getBeginLoc());
+        const Stmt *Inner = S.getRawStmt();
+        llvm::CanonicalLoopInfo *CLI =
+            EmitOMPCollapsedCanonicalLoopNest(Inner, 1);
+
+        llvm::OpenMPIRBuilder &OMPBuilder =
+            CGM.getOpenMPRuntime().getOMPBuilder();
+        // Add SIMD specific metadata
+        OMPBuilder.createSIMDLoop(DL, CLI);
+        return;
+      }
+    };
+    {
+      auto LPCRegion =
+          CGOpenMPRuntime::LastprivateConditionalRAII::disable(*this, S);
+      OMPLexicalScope Scope(*this, S, OMPD_unknown);
+      CGM.getOpenMPRuntime().emitInlinedDirective(*this, OMPD_simd,
+                                                  CodeGenIRBuilder);
+    }
+    return;
+  }
+
   ParentLoopDirectiveForScanRegion ScanRegion(*this, S);
   OMPFirstScanLoop = true;
   auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) {
     emitOMPSimdRegion(CGF, S, Action);
   };
   {
     auto LPCRegion =
         CGOpenMPRuntime::LastprivateConditionalRAII::disable(*this, S);

clang/test/OpenMP/simd_codegen_irbuilder.cpp

+// RUN: %clang_cc1 -fopenmp-enable-irbuilder -verify -fopenmp -fopenmp-version=45 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+struct S {
+  int a, b;
+};
+
+void simple(float *a, float *b) {
+  S s, *p;
+  int j = 0;
+#pragma omp simd
+  for (int i = 3; i < 32; i += 5) {
+    // llvm.access.group test
+    // CHECK: omp_loop.body:                                    ; preds = %omp_loop.cond
+    // CHECK-NEXT: call void @__captured_stmt.1(i32* %i, i32 %omp_loop.iv, %struct.anon.0* %agg.captured1)
+    // CHECK-NEXT: %3 = load float*, float** %b.addr, align 8, !llvm.access.group !3
+    // CHECK-NEXT: %4 = load i32, i32* %i, align 4, !llvm.access.group !3
+    // CHECK-NEXT: %idxprom = sext i32 %4 to i64
+    // CHECK-NEXT: %arrayidx = getelementptr inbounds float, float* %3, i64 %idxprom
+    // CHECK-NEXT: %5 = load float, float* %arrayidx, align 4, !llvm.access.group !3
+    // CHECK-NEXT: %a2 = getelementptr inbounds %struct.S, %struct.S* %s, i32 0, i32 0
+    // CHECK-NEXT: %6 = load i32, i32* %a2, align 4, !llvm.access.group !3
+    // CHECK-NEXT: %conv = sitofp i32 %6 to float
+    // CHECK-NEXT: %add = fadd float %5, %conv
+    // CHECK-NEXT: %7 = load %struct.S*, %struct.S** %p, align 8, !llvm.access.group !3
+    // CHECK-NEXT: %a3 = getelementptr inbounds %struct.S, %struct.S* %7, i32 0, i32 0
+    // CHECK-NEXT: %8 = load i32, i32* %a3, align 4, !llvm.access.group !3
+    // CHECK-NEXT: %conv4 = sitofp i32 %8 to float
+    // CHECK-NEXT: %add5 = fadd float %add, %conv4
+    // CHECK-NEXT: %9 = load float*, float** %a.addr, align 8, !llvm.access.group !3
+    // CHECK-NEXT: %10 = load i32, i32* %i, align 4, !llvm.access.group !3
+    // CHECK-NEXT: %idxprom6 = sext i32 %10 to i64
+    // CHECK-NEXT: %arrayidx7 = getelementptr inbounds float, float* %9, i64 %idxprom6
+    // CHECK-NEXT: store float %add5, float* %arrayidx7, align 4, !llvm.access.group !3
+    // llvm.loop test
+    // CHECK: %omp_loop.next = add nuw i32 %omp_loop.iv, 1
+    // CHECK-NEXT: br label %omp_loop.header, !llvm.loop !4
+    // CHECK: !4 = distinct !{!4, !5, !6}
+    // CHECK-NEXT: !5 = !{!"llvm.loop.parallel_accesses"}
+    // CHECK-NEXT: !6 = !{!"llvm.loop.vectorize.enable"}
+    a[i] = b[i] + s.a + p->a;
+  }
+}
 No newline at end of file

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

public:
   ///                    indicates that a heuristic should be used to determine
   ///                    the unroll-factor.
   /// \param UnrolledCLI If non-null, receives the CanonicalLoopInfo of the
   ///                    partially unrolled loop. Otherwise, uses loop metadata
   ///                    to defer unrolling to the LoopUnrollPass.
   void unrollLoopPartial(DebugLoc DL, CanonicalLoopInfo *Loop, int32_t Factor,
                          CanonicalLoopInfo **UnrolledCLI);
 
+  /// Add metadata to simd-ise a loop.
+  ///
+  /// \param DL   Debug location for instructions added by unrolling.
+  /// \param Loop The loop to simd-ise.
+  void createSIMDLoop(DebugLoc DL, CanonicalLoopInfo *Loop);
+
   /// Generator for '#omp flush'
   ///
   /// \param Loc The location where the flush directive was encountered
   void createFlush(const LocationDescription &Loc);
 
   /// Generator for '#omp taskwait'
   ///
   /// \param Loc The location where the taskwait directive was encountered.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

static void addLoopMetadata(CanonicalLoopInfo *Loop,
 
   append_range(NewLoopProperties, Properties);
   MDNode *LoopID = MDNode::getDistinct(Ctx, NewLoopProperties);
   LoopID->replaceOperandWith(0, LoopID);
 
   Latch->getTerminator()->setMetadata(LLVMContext::MD_loop, LoopID);
 }
 
+/// Attach metadata access.group to the load and store instructions of \p block
+static void addSIMDMetadata(BasicBlock *block,
+                            ArrayRef<Metadata *> Properties) {
+  for (auto &I : *block) {
+    if (isa<LoadInst>(&I) || isa<StoreInst>(&I)) {
+      Instruction *instr = dyn_cast<Instruction>(&I);
+      LLVMContext &C = instr->getContext();
+      MDNode *N = MDNode::get(C, MDString::get(C, ""));
+      instr->setMetadata("llvm.access.group", N);
+    }
+  }
+}
+
 void OpenMPIRBuilder::unrollLoopFull(DebugLoc, CanonicalLoopInfo *Loop) {
   LLVMContext &Ctx = Builder.getContext();
   addLoopMetadata(
       Loop, {MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll.enable")),
              MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll.full"))});
 }
 
 void OpenMPIRBuilder::unrollLoopHeuristic(DebugLoc, CanonicalLoopInfo *Loop) {
   LLVMContext &Ctx = Builder.getContext();
   addLoopMetadata(
       Loop, {
                 MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll.enable")),
             });
 }
 
+void OpenMPIRBuilder::createSIMDLoop(DebugLoc, CanonicalLoopInfo *Loop) {
+  LLVMContext &Ctx = Builder.getContext();
+  addLoopMetadata(
+      Loop,
+      {MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.parallel_accesses")),
+       MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.vectorize.enable"))});
+  BasicBlock *header = Loop->getHeader();
+  BasicBlock *cond = Loop->getCond();
+  BasicBlock *body = Loop->getBody();
+
+  addSIMDMetadata(header,
+                  {
+                      MDNode::get(Ctx, MDString::get(Ctx, "llvm.access.group")),
+                  });
+  addSIMDMetadata(cond,
+                  {
+                      MDNode::get(Ctx, MDString::get(Ctx, "llvm.access.group")),
+                  });
+  addSIMDMetadata(body,
+                  {
+                      MDNode::get(Ctx, MDString::get(Ctx, "llvm.access.group")),
+                  });
+}
+
 /// Create the TargetMachine object to query the backend for optimization
 /// preferences.
 ///
 /// Ideally, this would be passed from the front-end to the OpenMPBuilder, but
 /// e.g. Clang does not pass it to its CodeGen layer and creates it only when
 /// needed for the LLVM pass pipline. We use some default options to avoid
 /// having to pass too many settings from the frontend that probably do not
 /// matter.
 ///
 /// Currently, TargetMachine is only used sometimes by the unrollLoopPartial
 /// method. If we are going to use TargetMachine for more purposes, especially
 /// those that are sensitive to TargetOptions, RelocModel and CodeModel, it
 /// might become be worth requiring front-ends to pass on their TargetMachine,
 /// or at least cache it between methods. Note that while fontends such as Clang
 /// have just a single main TargetMachine per translation unit, "target-cpu" and
 /// "target-features" that determine the TargetMachine are per-function and can
 /// be overrided using __attribute__((target("OPTIONS"))).
 static std::unique_ptr<TargetMachine>
 createTargetMachine(Function *F, CodeGenOpt::Level OptLevel) {
   Module *M = F->getParent();