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

[Faisal Vali]

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.
>


I do likes your [1] below!  What it lacks in complexity it makes up
for in simplicity ;)
(and hopefully rectitude)

What i'd like to do is implement [1] - make sure it is reasonably
correct and then once that is
committed - if you want, I can submit the other codegen changes as optimization
patches (though between you and me, I shudder at the thought of me optimizing
codegen).

But more importantly, now that I have your attention i'll be pinging
you with the generic
lambda patch (sorry - I can tell you're incredibly busy - if only I
could get Doug to
3D-print the code for me ...  ;)

>>>>   2) CGF::EmitDeclRef now handles unused variables that are constant
>>>> expressions
>>>>       as long as they are mentioned within lambdas or found within the local
>>>> decl map.
>>>
>>> I don't think the added assertion is correct. For instance, this should be OK:
>>>
>>> void f() {
>>>   const thread_local int n = 0;
>>>   [] { return n; };
>>> }
>>>
>>> 'n' here, as an lvalue, is not a constant expression. But after
>>> applying an lvalue-to-rvalue conversion, it is a constant expression.
>>>
>>
>> OK. I instead use the assertion isUsableInConstantExpressions().
>> In addition for thread_locals, they are treated as static locals, so don't
>> make it into our uncaptured auto variable const used in lambda scenario.
>
> OK, that seems reasonable. You still seem to have a use of
> E->isCXX11ConstantExpr on line 1755. Should that also be
> isUsableInConstantExpressions()?
>
>>> Also, you can't rely on local constant-initialized variables being
>>> hoisted to globals. That's a non-conforming IR generation feature and
>>> can be disabled by -fno-merge-all-constants.
>>>
>>
>> I dealt with this by forcing the constant declaration to be emitted
>> as a static var if the no-merge option is on :(
>
> OK, the approach of emitting such variables as static constants if
> they're used within a lambda seems reasonable.[1]
>
> However... the MergeAllConstants check is not right in all cases. For
> instance, if the local variable is the NRVO variable, it won't have
> been emitted as a static local in this case. Instead, you could check
> whether the variable is in the map, and if not, call EmitStaticVarDecl
> or something like it. Please add an assert(VD->checkInitIsICE()) here,
> too. Also, the EmitStaticVarDecl call here is liable to assert (it
> checks that the variable is not already in LocalDeclMap, which it may
> be), so that will need a little more work.
>
>
> [1] It strikes me that you can extend this approach to address all of
> DR712 without any other changes, if you apply it to all variables that
> are not odr-used. That is, whenever we emit a DeclRefExpr referring to
> a decl that is not marked as used (or, in a lambda, is from a
> surrounding scope and not captured), emit an internal constant with
> its value and use that instead. The program cannot tell the
> difference, because if it could, the variable would have been marked
> as odr-used.
>
> Essentially the idea here is to just rely on Sema getting 'used'
> marking correct, rather than attempting to reinvent it in CodeGen. The
> other CodeGen changes in your patch still have some value, since
> they'll allow us to more directly emit the appropriate code in more
> cases, but they would no longer be necessary, because it would become
> impossible for CodeGen to emit a reference to a variable that is not
> odr-used.