r234499 - [CodeGen] When promoting a reference temporary to a global use the inner type to fold it.

[Benjamin Kramer]

On Fri, Apr 10, 2015 at 12:07 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, Apr 9, 2015 at 2:48 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>>
>>
>> > On 09.04.2015, at 23:05, Richard Smith <richard at metafoo.co.uk> wrote:
>> >
>> > On Thu, Apr 9, 2015 at 9:09 AM, Benjamin Kramer
>> > <benny.kra at googlemail.com> wrote:
>> > Author: d0k
>> > Date: Thu Apr  9 11:09:29 2015
>> > New Revision: 234499
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=234499&view=rev
>> > Log:
>> > [CodeGen] When promoting a reference temporary to a global use the inner
>> > type to fold it.
>> >
>> > The MaterializeTemporaryExpr can have a different type than the inner
>> > expression, miscompiling the constant. PR23165.
>> >
>> > Modified:
>> >     cfe/trunk/lib/CodeGen/CGExpr.cpp
>> >     cfe/trunk/test/CodeGenCXX/cxx0x-initializer-references.cpp
>> >
>> > Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=234499&r1=234498&r2=234499&view=diff
>> >
>> > ==============================================================================
>> > --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
>> > +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Thu Apr  9 11:09:29 2015
>> > @@ -309,12 +309,13 @@ createReferenceTemporary(CodeGenFunction
>> >          (M->getType()->isArrayType() || M->getType()->isRecordType())
>> > &&
>> >          CGF.CGM.isTypeConstant(M->getType(), true))
>> >
>> > This reference to M->getType() should be changed to Inner->getType()
>> > too. Consider a case like:
>> >
>> >   struct S { struct T { int a; } t; mutable int b; };
>> >   void f() { const S::T &r = S().t; }
>> >
>> > Here, we shouldn't emit the temporary as a global constant due to the
>> > mutable member.
>> >
>> > The same is probably true of the array type / record type check -- if
>> > the reference temporary itself is of array or reference type, and we're
>> > binding a reference to a non-array/non-reference subobject, we should
>> > probably still promote to a global.
>>
>> You're right. Changing this is tricky though without destroying the
>> optimization. If I remove the mutable from your test case the AST looks like
>> this:
>>
>>         `-MaterializeTemporaryExpr 0x7faab985f560 <col:30, col:34> 'const
>> struct S::T':'const struct S::T' lvalue extended by Var 0x7faab985eda0 'r'
>> 'const struct S::T &'
>>           `-ImplicitCastExpr 0x7faab985f548 <col:30, col:34> 'const struct
>> S::T':'const struct S::T' <NoOp>
>>             `-MemberExpr 0x7faab985f248 <col:30, col:34> 'struct
>> T':'struct S::T' .t 0x7faab985eaa0
>>               `-CXXTemporaryObjectExpr 0x7faab985f108 <col:30, col:32>
>> 'struct S' 'void (void)' zeroing
>>
>> Note the lack of const on the CXXTemporaryObjectExpr :(
>
>
> Hmm, right, the optimization is not correct with or without the 'mutable'
> because the temporary is not actually constant; the 'const' on M can be
> removed by a const_cast without UB. "Destroying the optimization" is the
> right thing to do here. We can still optimize in the case where Inner has a
> constant type.
>

Makes sense. Fixed in r234543.

- Ben

>>
>>
>> >
>> >        if (llvm::Constant *Init =
>> > -              CGF.CGM.EmitConstantExpr(Inner, M->getType(), &CGF)) {
>> > +              CGF.CGM.EmitConstantExpr(Inner, Inner->getType(), &CGF))
>> > {
>> >          auto *GV = new llvm::GlobalVariable(
>> >              CGF.CGM.getModule(), Init->getType(), /*isConstant=*/true,
>> >              llvm::GlobalValue::PrivateLinkage, Init, ".ref.tmp");
>> > -        GV->setAlignment(
>> > -
>> > CGF.getContext().getTypeAlignInChars(M->getType()).getQuantity());
>> > +        GV->setAlignment(CGF.getContext()
>> > +                             .getTypeAlignInChars(Inner->getType())
>> > +                             .getQuantity());
>> >          // FIXME: Should we put the new global into a COMDAT?
>> >          return GV;
>> >        }
>> >
>> > Modified: cfe/trunk/test/CodeGenCXX/cxx0x-initializer-references.cpp
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/cxx0x-initializer-references.cpp?rev=234499&r1=234498&r2=234499&view=diff
>> >
>> > ==============================================================================
>> > --- cfe/trunk/test/CodeGenCXX/cxx0x-initializer-references.cpp
>> > (original)
>> > +++ cfe/trunk/test/CodeGenCXX/cxx0x-initializer-references.cpp Thu Apr
>> > 9 11:09:29 2015
>> > @@ -1,5 +1,7 @@
>> >  // RUN: %clang_cc1 -std=c++11 -S -triple armv7-none-eabi -emit-llvm -o
>> > - %s | FileCheck %s
>> >
>> > +// CHECK: private constant { i8** } { i8** getelementptr inbounds ([3 x
>> > i8*], [3 x i8*]* @_ZTVN7PR2316510ChildClassE, i64 0, i64 2) }, align 4
>> > +
>> >  namespace reference {
>> >    struct A {
>> >      int i1, i2;
>> > @@ -79,3 +81,22 @@ namespace reference {
>> >    }
>> >
>> >  }
>> > +
>> > +namespace PR23165 {
>> > +struct AbstractClass {
>> > +  virtual void foo() const = 0;
>> > +};
>> > +
>> > +struct ChildClass : public AbstractClass {
>> > +  virtual void foo() const {}
>> > +};
>> > +
>> > +void helper(const AbstractClass &param) {
>> > +  param.foo();
>> > +}
>> > +
>> > +void foo() {
>> > +// CHECK: call void @_ZN7PR231656helperERKNS_13AbstractClassE(%{{.*}}
>> > bitcast ({ i8** }* @{{.*}} to %{{.*}}*))
>> > +  helper(ChildClass());
>> > +}
>> > +}
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >
>>
>