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

[John McCall]

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.

John.