[OpenMP][IR] Set correct alignment for internal variables

[erjin]

Summary

OpenMP runtime functions assume the pointers are aligned to sizeof(pointer), but it is being aligned incorrectly. Fix with the proper alignment in the IR builder.

[tianshilei1992]

It looks this issue will not just affect this single variable. It needs to be fixed in getOrCreateInternalVariable.

[erjin]

Thanks for the review! I have made changes based on the comment. Specify the alignment while making these internal variables.

[gchatelet]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4496–4499

You can go for the slightly more readable version below

const DataLayout &DL = M.getDataLayout();
const llvm::Align TypeAlign = DL.getABITypeAlign(Ty);
const llvm::Align PtrAlign = DL.getPointerABIAlignment(AddressSpace);
GV->setAlignment(std::max(TypeAlign, PtrAlign));

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

2753–2754

GV->getAlignment() should disappear. Can you use the following lines instead?

if(const llvm::MaybeAlign Alignment = GV->getAlign()) {
  EXPECT_EQ(*Alignment, PtrAlign);
}

[erjin]

Thanks for the suggestions. Made changes based on them.

[tianshilei1992]

I think the diff is based on your previous version instead of trunk. Can you rebase it?

[erjin]

Rebased. Thanks

[tianshilei1992]

The parent version for clang/lib/CodeGen/CGOpenMPRuntime.cpp is still not right.

[erjin]

Rebased with arc. Hope it works now

[erjin]

Ping. :)

[tianshilei1992]

LGTM

clang/lib/CodeGen/CGOpenMPRuntime.cpp

CGF.EmitStoreOfScalar(ThreadID,
                         CGF.MakeAddrLValue(ThreadIDTemp, Int32Ty));
 
   return ThreadIDTemp;
 }
 
 llvm::Value *CGOpenMPRuntime::getCriticalRegionLock(StringRef CriticalName) {
   std::string Prefix = Twine("gomp_critical_user_", CriticalName).str();
   std::string Name = getName({Prefix, "var"});
-  llvm::GlobalVariable *G = OMPBuilder.getOrCreateInternalVariable(KmpCriticalNameTy, Name);
-  llvm::Align PtrAlign = OMPBuilder.M.getDataLayout().getPointerABIAlignment(G->getAddressSpace());
-  if (PtrAlign > llvm::Align(G->getAlignment()))
-    G->setAlignment(PtrAlign);
-  return G;
+  return OMPBuilder.getOrCreateInternalVariable(KmpCriticalNameTy, Name);
 }
 
 namespace {
 /// Common pre(post)-action for different OpenMP constructs.
 class CommonActionTy final : public PrePostActionTy {
   llvm::FunctionCallee EnterCallee;
   ArrayRef<llvm::Value *> EnterArgs;
   llvm::FunctionCallee ExitCallee;

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

if (Elem.second) {
     // TODO: investigate the appropriate linkage type used for the global
     // variable for possibly changing that to internal or private, or maybe
     // create different versions of the function for different OMP internal
     // variables.
     auto *GV = new GlobalVariable(
         M, Ty, /*IsConstant=*/false, GlobalValue::CommonLinkage,
         Constant::getNullValue(Ty), Elem.first(),
         /*InsertBefore=*/nullptr, GlobalValue::NotThreadLocal, AddressSpace);
-    GV->setAlignment(M.getDataLayout().getABITypeAlign(Ty));
+    const DataLayout &DL = M.getDataLayout();
+    const llvm::Align TypeAlign = DL.getABITypeAlign(Ty);
+    const llvm::Align PtrAlign = DL.getPointerABIAlignment(AddressSpace);
+    GV->setAlignment(std::max(TypeAlign, PtrAlign));
     Elem.second = GV;
   }
 
   return Elem.second;
 }
 
 Value *OpenMPIRBuilder::getOMPCriticalRegionLock(StringRef CriticalName) {
   std::string Prefix = Twine("gomp_critical_user_", CriticalName).str();

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
   }
   EXPECT_NE(CriticalEndCI, nullptr);
   EXPECT_EQ(CriticalEndCI->arg_size(), 3U);
   EXPECT_TRUE(isa<GlobalVariable>(CriticalEndCI->getArgOperand(0)));
   EXPECT_EQ(CriticalEndCI->getArgOperand(1), CriticalEntryCI->getArgOperand(1));
   PointerType *CriticalNamePtrTy =
       PointerType::getUnqual(ArrayType::get(Type::getInt32Ty(Ctx), 8));
   EXPECT_EQ(CriticalEndCI->getArgOperand(2), CriticalEntryCI->getArgOperand(2));
-  EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
+  GlobalVariable *GV =
+      dyn_cast<GlobalVariable>(CriticalEndCI->getArgOperand(2));
+  ASSERT_NE(GV, nullptr);
+  EXPECT_EQ(GV->getType(), CriticalNamePtrTy);
+  const DataLayout &DL = M->getDataLayout();
+  const llvm::Align TypeAlign = DL.getABITypeAlign(CriticalNamePtrTy);
+  const llvm::Align PtrAlign = DL.getPointerABIAlignment(GV->getAddressSpace());
+  if (const llvm::MaybeAlign Alignment = GV->getAlign())
+    EXPECT_EQ(*Alignment, std::max(TypeAlign, PtrAlign));
 }
 
 TEST_F(OpenMPIRBuilderTest, OrderedDirectiveDependSource) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   F->setName("func");
   IRBuilder<> Builder(BB);