[PATCH] Implements DR 712 (odr-use, potential results of an expression)

[Faisal Vali]

On Thu, Jul 11, 2013 at 4:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, Jul 11, 2013 at 12:37 PM, Faisal Vali <faisalv at gmail.com> wrote:
>> On Wed, Jul 10, 2013 at 6:57 PM, John McCall <rjmccall at apple.com> wrote:
>>> On Jul 10, 2013, at 4:51 PM, Faisal Vali <faisalv at gmail.com> wrote:
>>>> On Wed, Jul 10, 2013 at 4:56 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>>> On Wed, Jul 10, 2013 at 10:06 AM, Faisal Vali <faisalv at gmail.com> wrote:
>>>>>> On Wed, Jun 12, 2013 at 7:02 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>>>>> On Sun, Jun 9, 2013 at 12:47 PM, Faisal Vali <faisalv at gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jun 7, 2013 at 5:21 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Jun 4, 2013 at 9:19 PM, Faisal Vali <faisalv at gmail.com> wrote:
>>>>>>>>>> OK - this patch adds the following:
>>>>>>>>>> 0) Some codegen tests, and additional non-codegen tests.
>>>>>>>>>> 1) If the pointer to member expression can be entirely folded into a
>>>>>>>>>> constant, do so.
>>>>>>>>>> 2) If the object expression in a member access (be it through dot, or
>>>>>>>>>> pointer to member) is a constant
>>>>>>>>>>     but the containing expression could not be folded into a constant -
>>>>>>>>>> do
>>>>>>>>>> not emit the object as a constant
>>>>>>>>>>     - instead get the variable's address using
>>>>>>>>>> getStaticLocalDeclAddress
>>>>>>>>>> (it appears that constants are
>>>>>>>>>>       hoisted out as globals in llvm IR) so that getelementptr has a
>>>>>>>>>> pointer to work with.
>>>>>>>>>>
>>>>>>>>>> Am patiently awaiting feedback - because if i am on the right track,
>>>>>>>>>> hoping
>>>>>>>>>> i can get this committed soon; if I am way off, I would like to walk
>>>>>>>>>> away
>>>>>>>>>> from this, so that I can return to working on generic lambdas ;)
>>>>>>>>>
>>>>>>>>> The Sema part looks good.
>>>>>>>>>
>>>>>>>> OK.  No changes made to that portion, in this patch.
>>>>>>>>
>>>>>>>>> For the CodeGen part, I think you should approach the problem somewhat
>>>>>>>>> differently. CodeGen emits expressions as RValues or LValues based on
>>>>>>>>> how they are used, not based on whether the expressions themselves are
>>>>>>>>> rvalues or lvalues, and in particular, an operand of an
>>>>>>>>> lvalue-to-rvalue conversion is typically emitted directly as an
>>>>>>>>> rvalue. So if you ensure that RValue emission never actually performs
>>>>>>>>> a load in the cases which are not odr-uses, then you should be OK.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> One problem is that, in some cases, the rvalue emission defers to
>>>>>>>>> lvalue emission; emitting pointer-to-member access is one of those
>>>>>>>>> cases. I think what you should do here is to teach the
>>>>>>>>> ScalarExprEmitter to try to evaluate the right-hand side of the
>>>>>>>>> pointer-to-member expression, and emit a direct field access if it
>>>>>>>>> can.
>>>>>>>>>
>>>>>>>>
>>>>>>>> As per your suggestions, I have reworked the codegen portion as following:
>>>>>>>>  1) CGF::EmitIgnoredExpr checks to see if it is a constant expression, and
>>>>>>>> then
>>>>>>>>      defers to RValue Emission (at least i think my change does that, am I
>>>>>>>> correct?)
>>>>>>>
>>>>>>> Please don't use isCXX11ConstantExpr here; at the IR generation level,
>>>>>>> we should give the same behavior to any expression that we can
>>>>>>> constant fold without side-effects, whether or not it's a constant
>>>>>>> expression.
>>>>>>>
>>>>>>> If the expression is constant, you don't need to emit it at all. But
>>>>>>> this change isn't correct. Consider:
>>>>>>>
>>>>>>>  struct S { static const int n = 0; };
>>>>>>>  void doit(bool b, int k) { b ? k : S::n; }
>>>>>>>
>>>>>>> Since the discarded expression here isn't a constant expression,
>>>>>>> you'll emit a reference to S::n, which is not odr-used here.
>>>>>>>
>>>>>>> Instead, we could emit every discarded-value expression as an rvalue,
>>>>>>> in C++11 onwards. The extra lvalue-to-rvalue conversion is detectable
>>>>>>> iff the expression is volatile-qualified, which is exactly the
>>>>>>> situation in which we're required to emit an lvalue-to-rvalue
>>>>>>> conversion anyway.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> OK. I removed the IsCXX11ConstantExpr check, and if C++11 option is on,
>>>>>> we emit as an Rvalue.
>>>>>>
>>>>>> Also, in C++11 mode, this now emits 'load volatile' for this test and it fails -
>>>>>> is this the correct behavior ?
>>>>>> thoughts?
>>>>>>
>>>>>> volatile int& refcall();
>>>>>> // CHECK: define void @_Z2f2PVi
>>>>>> // CHECK-NOT: load volatile
>>>>>> // CHECK: ret
>>>>>> void f2(volatile int *x) {
>>>>>>  // We shouldn't perform the load in these cases.
>>>>>>  refcall();
>>>>>>  1 ? refcall() : *x;
>>>>>> }
>>>>>
>>>>> Hmm, OK, I was wrong to claim that we could emit every discarded-value
>>>>> expression as an rvalue; this test is correct. Here's an alternative:
>>>>> add an expression visitor for EmitIgnoredExpr to use, that implements
>>>>> the rules of 3.2/2 for discarded value expressions: Walk over
>>>>> conditionals, pointers-to-members, parens, and so on, evaluating an
>>>>> expression for its side-effects only. If you hit a DeclRefExpr, don't
>>>>> emit it. But see [1] below.
>>>
>>> Right, I agree that causing EmitIgnoredExpr to properly ignore l-values
>>> is the right way to go.  You can make more general than the 3.2/2 rules;
>>> you just need to make it at least as good.
>>>
>>
>> Attached is a patch that uses Richard's suggestion to turn non-odr-used
>> constants that Codegen tries to emit as LValues into hoisted static variables
>> that can be emitted as LValues.
>> It passes all the regression tests on my windows box, that passed
>> prior to its application - and passes the new tests.
>
> It's wasteful to call tryEmitAsConstant and discard the result.

OK.  I moved it into an assert - I believe it has (observable?)
side-effects though -
so perhaps I should just get rid of it?


> Also,
> this still seems vulnerable to EmitStaticVarDecl asserting, if the
> variable has previously been emitted but not as a static local. It's
> probably not easy to trigger this, because you'd need the variable to
> be emitted but not be odr-used, but it seems possible. Maybe something
> like this would do the trick:
>
>   struct X { int n; };
>   X f() {
>     constexpr X x = { 0 };
>     [] { return x.n; } ();
>     return x;
>   }
>

I'm afraid I still don't understand your concerns here.  The block
of code that contains the proposed changes is guarded by a check to
LocalDeclMap.lookup, which has to fail for these changes to
matter.  Doesn't that preclude the LocalDeclMap assertion
from being violated within EmitStaticVarDecl - what am I missing?

> This may also fall foul of your "isUsed" check; you should check for
> the variable either being unused, or referring to an uncaptured
> enclosing local, not just for it being unused.
>

Yes - you are absolutely correct - I reworded the assertion.  Is it still
incorrect?  What else can I assert here?

assert(((!VD->isUsed(false) || E->refersToEnclosingLocal()) &&
                  tryEmitAsConstant(const_cast<DeclRefExpr*>(E)))   &&
             "Must be either unused, or a non-captured constant expression");

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DR712--submit-make-static-cleaner.patch
Type: application/octet-stream
Size: 19786 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130711/8129a095/attachment.obj>