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

[Richard Smith]

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.


> - 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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150409/ef5401eb/attachment.html>