[CodeGen] Pessimize aliasing for union members (and may-alias) objects

[kparzysz]

Summary

Use the TBAA info of the omnipotent char for these objects.

[kparzysz]

The diff is the same as before, the change is in the subscribers: from llvm-commits to cfe-commits.

[kparzysz]

Previously, the variable mayAlias in EmitLValueForField could have been false even if the base LValue had may-alias flag set. Make sure that mayAlias reflects the base aliasing.

This fixed the C++ testcase and it's now included in the patch.

[hfinkel]

test/CodeGen/union-tbaa1.c

44 ↗

(On Diff #99918)

Please check the path offset?

test/CodeGenCXX/union-tbaa2.cpp

45 ↗

(On Diff #99918)

Please check the path offset.

[kparzysz]

test/CodeGen/union-tbaa1.c

44 ↗

(On Diff #99918)

What needs to be checked exactly? The offset will be 0, do you want to check for that?

[hfinkel]

test/CodeGen/union-tbaa1.c

44 ↗

(On Diff #99918)

Yes, because if it's not zero, this won't have the desired effect.

[kparzysz]

Checking for offset 0 in the TBAA info.

[rjmccall]

lib/CodeGen/CGExpr.cpp

1436 ↗

(On Diff #99939)

Hmm. Should we be constructing a struct-path TBAA at all if the base may alias, as opposed to just using 'char'?

[kparzysz]

lib/CodeGen/CGExpr.cpp

1436 ↗

(On Diff #99939)

I did that and got a verifier error: "Old-style TBAA is no longer allowed, use struct-path TBAA instead. fatal error: error in backend: Broken function found, compilation aborted!"

[rjmccall]

lib/CodeGen/CGExpr.cpp

1436 ↗

(On Diff #99939)

Oh, I haven't been paying enough attention to LLVM's TBAA changes. It looks like they've canonicalized on always using the struct-access-style TBAA metadata, which is fine; the point is that we should be using whatever tag we would use for dereferencing a simple char*, which I believe just means passing true for ConvertTypeToTag to DecorateInstructionWithTBAA .

[kparzysz]

lib/CodeGen/CGExpr.cpp

1436 ↗

(On Diff #99939)

With that change I'm getting another error:

Access type node must be a valid scalar type
  store <4 x double> %call2, <4 x double>* %b, align 32, !tbaa !8
!8 = !{!9, !9, i64 0}
!9 = !{!4, !4, i64 0}
fatal error: error in backend: Broken function found, compilation aborted!

[rjmccall]

lib/CodeGen/CGExpr.cpp

1436 ↗

(On Diff #99939)

Sorry, I see now that what I said was unclear. If you do this, you need to pass the result of getTBAAInfo(CharTy) down as the argument to DecorateInstructionWithTBAA instead of building of a TBAA struct tag.

[kparzysz]

Pass char TBAA directly to DecorateInstructionWithTBAA and pass true for ConvertTypeToTag in such cases.

[rjmccall]

Thanks, that looks great.

cfe/trunk/lib/CodeGen/CGExpr.cpp

llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
 
   llvm::LoadInst *Load = Builder.CreateLoad(Addr, Volatile);
   if (isNontemporal) {
     llvm::MDNode *Node = llvm::MDNode::get(
         Load->getContext(), llvm::ConstantAsMetadata::get(Builder.getInt32(1)));
     Load->setMetadata(CGM.getModule().getMDKindID("nontemporal"), Node);
   }
   if (TBAAInfo) {
-    llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo,
-                                                      TBAAOffset);
-    if (TBAAPath)
-      CGM.DecorateInstructionWithTBAA(Load, TBAAPath,
-                                      false /*ConvertTypeToTag*/);
+    bool MayAlias = BaseInfo.getMayAlias();
+    llvm::MDNode *TBAA = MayAlias
+        ? CGM.getTBAAInfo(getContext().CharTy)
+        : CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, TBAAOffset);
+    if (TBAA)
+      CGM.DecorateInstructionWithTBAA(Load, TBAA, MayAlias);
   }
 
   if (EmitScalarRangeCheck(Load, Ty, Loc)) {
     // In order to prevent the optimizer from throwing away the check, don't
     // attach range metadata to the load.
   } else if (CGM.getCodeGenOpts().OptimizationLevel > 0)
     if (llvm::MDNode *RangeInfo = getRangeForLoadFromType(Ty))
       Load->setMetadata(llvm::LLVMContext::MD_range, RangeInfo);
void CodeGenFunction::EmitStoreOfScalar(llvm::Value *Value, Address Addr,
   llvm::StoreInst *Store = Builder.CreateStore(Value, Addr, Volatile);
   if (isNontemporal) {
     llvm::MDNode *Node =
         llvm::MDNode::get(Store->getContext(),
                           llvm::ConstantAsMetadata::get(Builder.getInt32(1)));
     Store->setMetadata(CGM.getModule().getMDKindID("nontemporal"), Node);
   }
   if (TBAAInfo) {
-    llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo,
-                                                      TBAAOffset);
-    if (TBAAPath)
-      CGM.DecorateInstructionWithTBAA(Store, TBAAPath,
-                                      false /*ConvertTypeToTag*/);
+    bool MayAlias = BaseInfo.getMayAlias();
+    llvm::MDNode *TBAA = MayAlias
+        ? CGM.getTBAAInfo(getContext().CharTy)
+        : CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, TBAAOffset);
+    if (TBAA)
+      CGM.DecorateInstructionWithTBAA(Store, TBAA, MayAlias);
   }
 }
 
 void CodeGenFunction::EmitStoreOfScalar(llvm::Value *value, LValue lvalue,
                                         bool isInit) {
   EmitStoreOfScalar(value, lvalue.getAddress(), lvalue.isVolatile(),
                     lvalue.getType(), lvalue.getBaseInfo(),
                     lvalue.getTBAAInfo(), isInit, lvalue.getTBAABaseType(),

 
 LValue CodeGenFunction::EmitLValueForField(LValue base,
                                            const FieldDecl *field) {
   LValueBaseInfo BaseInfo = base.getBaseInfo();
   AlignmentSource fieldAlignSource =
     getFieldAlignmentSource(BaseInfo.getAlignmentSource());
   LValueBaseInfo FieldBaseInfo(fieldAlignSource, BaseInfo.getMayAlias());
 
+  const RecordDecl *rec = field->getParent();
+  if (rec->isUnion() || rec->hasAttr<MayAliasAttr>())
+    FieldBaseInfo.setMayAlias(true);
+  bool mayAlias = FieldBaseInfo.getMayAlias();
+
   if (field->isBitField()) {
     const CGRecordLayout &RL =
       CGM.getTypes().getCGRecordLayout(field->getParent());
     const CGBitFieldInfo &Info = RL.getBitFieldInfo(field);
     Address Addr = base.getAddress();
     unsigned Idx = RL.getLLVMFieldNo(field);
     if (Idx != 0)
       // For structs, we GEP to the field that the record layout suggests.
       Addr = Builder.CreateStructGEP(Addr, Idx, Info.StorageOffset,
                                      field->getName());
     // Get the access type.
     llvm::Type *FieldIntTy =
       llvm::Type::getIntNTy(getLLVMContext(), Info.StorageSize);
     if (Addr.getElementType() != FieldIntTy)
       Addr = Builder.CreateElementBitCast(Addr, FieldIntTy);
 
     QualType fieldType =
       field->getType().withCVRQualifiers(base.getVRQualifiers());
     return LValue::MakeBitfield(Addr, Info, fieldType, FieldBaseInfo);
   }
 
-  const RecordDecl *rec = field->getParent();
   QualType type = field->getType();
-
-  bool mayAlias = rec->hasAttr<MayAliasAttr>();
-
   Address addr = base.getAddress();
   unsigned cvr = base.getVRQualifiers();
   bool TBAAPath = CGM.getCodeGenOpts().StructPathTBAA;
   if (rec->isUnion()) {
     // For unions, there is no pointer adjustment.
     assert(!type->isReferenceType() && "union has reference member");
     // TODO: handle path-aware TBAA for union.
     TBAAPath = false;

cfe/trunk/test/CodeGen/union-tbaa1.c

+// RUN: %clang_cc1 %s -triple hexagon-unknown-elf -O2 -emit-llvm -o - | FileCheck %s
+
+typedef union __attribute__((aligned(4))) {
+  unsigned short uh[2];
+  unsigned uw;
+} vect32;
+
+void bar(vect32 p[][2]);
+
+// CHECK-LABEL: define void @fred
+void fred(unsigned Num, int Vec[2], int *Index, int Arr[4][2]) {
+  vect32 Tmp[4][2];
+// Generate tbaa for the load of Index:
+// CHECK: load i32, i32* %Index{{.*}}tbaa
+// But no tbaa for the two stores:
+// CHECK: %uw[[UW1:[0-9]*]] = getelementptr
+// CHECK: store{{.*}}%uw[[UW1]]
+// CHECK: tbaa ![[OCPATH:[0-9]+]]
+// There will be a load after the store, and it will use tbaa. Make sure
+// the check-not above doesn't find it:
+// CHECK: load
+  Tmp[*Index][0].uw = Arr[*Index][0] * Num;
+// CHECK: %uw[[UW2:[0-9]*]] = getelementptr
+// CHECK: store{{.*}}%uw[[UW2]]
+// CHECK: tbaa ![[OCPATH]]
+  Tmp[*Index][1].uw = Arr[*Index][1] * Num;
+// Same here, don't generate tbaa for the loads:
+// CHECK: %uh[[UH1:[0-9]*]] = bitcast %union.vect32
+// CHECK: %arrayidx[[AX1:[0-9]*]] = getelementptr{{.*}}%uh[[UH1]]
+// CHECK: load i16, i16* %arrayidx[[AX1]]
+// CHECK: tbaa ![[OCPATH]]
+// CHECK: store
+  Vec[0] = Tmp[*Index][0].uh[1];
+// CHECK: %uh[[UH2:[0-9]*]] = bitcast %union.vect32
+// CHECK: %arrayidx[[AX2:[0-9]*]] = getelementptr{{.*}}%uh[[UH2]]
+// CHECK: load i16, i16* %arrayidx[[AX2]]
+// CHECK: tbaa ![[OCPATH]]
+// CHECK: store
+  Vec[1] = Tmp[*Index][1].uh[1];
+  bar(Tmp);
+}
+
+// CHECK-DAG: ![[CHAR:[0-9]+]] = !{!"omnipotent char"
+// CHECK-DAG: ![[OCPATH]] = !{![[CHAR]], ![[CHAR]], i64 0}

cfe/trunk/test/CodeGenCXX/union-tbaa2.cpp

+// RUN: %clang_cc1 %s -O2 -std=c++11 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -target-feature +sse4.2 -target-feature +avx -emit-llvm -o - | FileCheck %s
+
+// Testcase from llvm.org/PR32056
+
+extern "C" int printf (const char *__restrict __format, ...);
+
+typedef double __m256d __attribute__((__vector_size__(32)));
+
+static __inline __m256d __attribute__((__always_inline__, __nodebug__,
+                                       __target__("avx")))
+_mm256_setr_pd(double __a, double __b, double __c, double __d) {
+  return (__m256d){ __a, __b, __c, __d };
+}
+
+struct A {
+  A () {
+// Check that the TBAA information generated for the stores to the
+// union members is based on the omnipotent char.
+// CHECK: store <4 x double>
+// CHECK: tbaa ![[OCPATH:[0-9]+]]
+// CHECK: store <4 x double>
+// CHECK: tbaa ![[OCPATH]]
+// CHECK: call
+    a = _mm256_setr_pd(0.0, 1.0, 2.0, 3.0);
+    b = _mm256_setr_pd(4.0, 5.0, 6.0, 7.0);
+  }
+
+  const double *begin() { return c; }
+  const double *end() { return c+8; }
+
+  union {
+    struct { __m256d a, b; };
+    double c[8];
+  };
+};
+
+int main(int argc, char *argv[]) {
+  A a;
+  for (double value : a)
+    printf("%f ", value);
+  return 0;
+}
+
+// CHECK-DAG: ![[CHAR:[0-9]+]] = !{!"omnipotent char"
+// CHECK-DAG: ![[OCPATH]] = !{![[CHAR]], ![[CHAR]], i64 0}