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

[Richard Smith]

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.

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