[PATCH] D133578: [OpenMP][OMPIRBuilder] Add generation of SIMD align assumptions to OMPIRBuilder

Wed Oct 5 06:12:30 PDT 2022

jdoerfert added inline comments.

================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2640
       if (UseOMPIRBuilder) {
+        llvm::DenseMap<llvm::Value *, llvm::Value *> AlignedVars;
         // Emit the associated statement and get its loop representation.
----------------
Where is this map populated?

================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:621
+  /// \param Loop        The loop to simd-ize.
+  /// \param AlignedVars The map of the variables which need to aligned with.
+  /// \param IfCond      The value which corresponds to the if clause
----------------
> The map of the variables which need to aligned with.

This reads weird. Can you rephrase

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3033
+           "Value which needs to be aligned must represented by alloca "
+           "instruction");
+    assert(AlignedItem.second &&
----------------
Why use Value as type and then assert it's an Alloca? Use a map with Alloca as key.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3037
+    Value *AlignPtrInstruction = nullptr;
+    AllocaInst *AllocaInstruction = dyn_cast<AllocaInst>(AlignedItem.first);
+    Builder.SetInsertPoint(CanonicalLoop->getPreheader()->getTerminator());
----------------
This won't be necessary once you change the type but as a hint, `cast` is the right choice after an `isa`.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3049
+    assert(AlignPtrInstruction &&
+           "Aligned variables must be either pointer or array type");
+
----------------
Is Alloca of an array tested? I would not have assumed two different paths here. GEP on the alloca makes an assumption about the alloca, load of the alloca makes an assumption about the allocated type. If anything, this distinction should be based on the "isArray" property, not the allocated type. I mean, `alloca i32, 4` and `alloca [4xi32]` are different beasts. I fear we apply the handling of the former to the latter but I didn't check.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2976

+  const int DefaultAlignment = 16;
+
----------------
domada wrote:
> jdoerfert wrote:
> > domada wrote:
> > > jdoerfert wrote:
> > > > domada wrote:
> > > > > jdoerfert wrote:
> > > > > > This doesn't work. Use the data layout for any default values please.
> > > > > I have used pointer ABI alignment as the default value. Is it ok?
> > > > > Clang has separate method to calculate OpenMP default alignment defined in TargetInfo class ( [[ https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 |  link ]])
> > > > > Unfortunately, there is no simple replacement of this function in OMPIRBuilder or in Flang driver. There are some requests to expose TargetInfo data in Clang-independent manner: [[ https://discourse.llvm.org/t/rfc-targetinfo-library/64342 | RFC1 ]], [[https://discourse.llvm.org/t/complex-to-libm-conversion-abi-issues/65131 |RFC2]] , but this issue is not solved.
> > > > Let's step back:
> > > > 1) User tells us the alignment via `aligned(X:64)`. This should populate the map in Clang, I would assume.
> > > > 2) User tells us the alignment "implicitly" via `align(Y)`. Again, clang should populate the map with the default/preferred alignment of the type. You can use the function you linked, or use the data layout API to get the preferred alignment.
> > > > 3) User tells us nothing. We should pick the default or, don't annotate anything. If we go with defaults, you need to look up the default for the address space but I don't know why we would annotate anything not provided by the user.
> > > > 
> > > > 
> > > Ad 1) Agree
> > > Ad 2) OK, I will add assertion to ensure that the map is correctly populated with the default/preferred alignment if the user specifies only: `align(Y)`.
> > > 
> > > I was thinking that OMPIRBuilder should pick the default value if the user specifies `align(Y)`. Thanks for clarification. 
> > > 
> > > Ad 3)  I assume that if the user tells us nothing we should not annotate anything. I will expect empty map and no generated annotations. Currently we do not generate any annotations if there is no align clause.
> > > I was thinking that OMPIRBuilder should pick the default value if the user specifies align(Y). Thanks for clarification.
> > 
> > That works with me, but you need the actual type. So the Value is not sufficient, you need `&X and double` to ask DataLayout for the preferred type. For now you can use the clang functionality or pass the type and use DL.
> > 
> > > I assume that if the user tells us nothing we should not annotate anything. 
> > 
> > Agreed.
> >> I was thinking that OMPIRBuilder should pick the default value if the user specifies align(Y). Thanks for clarification.
> > That works with me, but you need the actual type. So the Value is not sufficient, you need &X and double to ask DataLayout for the preferred type. For now you can use the clang functionality or pass the type and use DL.
> 
> I rely on Clang functionality and I assume that the Clang always sets the size(default or specified by the user). That's why I added additional `assert` to applySimd function  to be sure that Clang always sets the alignment value.
> 
> Currently Clang does not use the actual type to calculate the default simd alignment: https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 . IMO it's better to rely on Clang and it's functionality which is widely tested.
> Currently Clang does not use the actual type to calculate the default simd alignment: https://clang.llvm.org/doxygen/classclang_1_1TargetInfo.html#af0c75e3288adc13601732329c44db147 . IMO it's better to rely on Clang and it's functionality which is widely tested.

You basically say clang is using a sub-optimal choice, let's rely on that as it is tested. I'm fine with that, but it seems odd.

================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1858
+  // Check if variables are correctly aligned
+  for (auto &Instr : LoopPreheader->getInstList()) {
+    if (isa<AssumeInst>(Instr)) {
----------------

================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1859
+  for (auto &Instr : LoopPreheader->getInstList()) {
+    if (isa<AssumeInst>(Instr)) {
+      auto AssumeInstruction = dyn_cast<AssumeInst>(&Instr);
----------------

================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1860
+    if (isa<AssumeInst>(Instr)) {
+      auto AssumeInstruction = dyn_cast<AssumeInst>(&Instr);
+      if (AssumeInstruction->getNumTotalBundleOperands()) {
----------------

================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1871
+  }
+}
+TEST_F(OpenMPIRBuilderTest, ApplySimdlen) {
----------------
This doesn't actually test that 2 assumes with alignments have been found, one for each alloca.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133578/new/

https://reviews.llvm.org/D133578