[OpenCL] Fix for TBAA information of pointer after addresspacecast

[romanovvlad]

Summary

When addresspacecast is generated resulting pointer should preserve TBAA information from original value.

[rjmccall]

LGTM. Although this also fixes bugs with unaligned pointers that you might consider adding tests for.

[romanovvlad]

What bugs with unaligned pointers do you mean?

[rjmccall]

The code was dropping all the original l-value information, which includes alignment. So if you start with an l-value that refers to under-aligned memory (or over-aligned), we were losing that information on the resulting l-value. The test case would be something like:

struct __attribute__((packed)) A { int x; };
int test(__global A *a) {
  return static_cast<__generic A &>(*a).x; // this load should have alignment 1
}

[romanovvlad]

Added new test case + formatting.

[rjmccall]

Oh, sorry, that wasn't a good example because that's still the natural alignment for A. It should be static_cast<__generic int &>(GPtr->X).

[romanovvlad]

I'm getting the same IR for both examples you provided. And for both load of X value is aligned on 4 bytes wo patch and on 1 byte with.

[rjmccall]

In D55262#1321048, @romanovvlad wrote:

I'm getting the same IR for both examples you provided. And for both load of X value is aligned on 4 bytes wo patch and on 1 byte with.

That's odd, because the natural type alignment for A ought to be 1, since it's a packed type. What does the AST look like?

[romanovvlad]

Sorry, I was compiling the first version two times.
With second example clang crashes :)

[rjmccall]

In D55262#1321641, @romanovvlad wrote:

Yes it's 1, but after addrspacecast alignment becomes not struct A but alignment of the pointer

Oh! Yes, that's even more broken than I thought.

lib/CodeGen/CGExpr.cpp

4272 ↗

(On Diff #176850)

This call to getAddressSpace() doesn't look right: we just constructed DestTy as an unqualified pointer type. It needs to be E->getType().getAddressSpace().

[romanovvlad]

Backtrace:

stack5 KBDownload

[rjmccall]

Sorry. Can you post the result of passing -ast-dump instead of -emit-llvm to the compiler?

[rjmccall]

In D55262#1321682, @rjmccall wrote:

Sorry. Can you post the result of passing -ast-dump instead of -emit-llvm to the compiler?

Actually, nevermind, I don't need that. If you can fix the address-space issue I pointed out, I think this is ready to go.

[rjmccall]

Thanks, LGTM.

[romanovvlad]

Could you merge it? I have no rights to do it.

[romanovvlad]

@Anastasia Could you, please, merge the patch if it's OK with you?

cfe/trunk/lib/CodeGen/CGExpr.cpp

case CK_LValueBitCast: {
     return MakeAddrLValue(V, E->getType(), LV.getBaseInfo(),
                           CGM.getTBAAInfoForSubobject(LV, E->getType()));
   }
   case CK_AddressSpaceConversion: {
     LValue LV = EmitLValue(E->getSubExpr());
     QualType DestTy = getContext().getPointerType(E->getType());
     llvm::Value *V = getTargetHooks().performAddrSpaceCast(
         *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
-        DestTy.getAddressSpace(), ConvertType(DestTy));
-    return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+        E->getType().getAddressSpace(), ConvertType(DestTy));
+    return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
+                          E->getType(), LV.getBaseInfo(), LV.getTBAAInfo());
   }
   case CK_ObjCObjectLValueCast: {
     LValue LV = EmitLValue(E->getSubExpr());
     Address V = Builder.CreateElementBitCast(LV.getAddress(),
                                              ConvertType(E->getType()));
     return MakeAddrLValue(V, E->getType(), LV.getBaseInfo(),
                           CGM.getTBAAInfoForSubobject(LV, E->getType()));
   }

cfe/trunk/test/CodeGenOpenCLCXX/address-space-deduction2.cl

+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s
+
+class P {
+public:
+  P(const P &Rhs) = default;
+
+  long A;
+  long B;
+};
+
+void foo(__global P *GPtr) {
+// CHECK: call void @llvm.memcpy{{.*}}, {{.*}}, i32 16
+  P Val = GPtr[0];
+}
+
+struct __attribute__((packed)) A { int X; };
+int test(__global A *GPtr) {
+// CHECK: {{.*}} = load i32, {{.*}}, align 1
+  return static_cast<__generic A &>(*GPtr).X;
+}