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

[Richard Smith]

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

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.

> Is this safe to commit before I implemented the visitor in EmitIgnoredExpr?
> Is that still necessary?

That is no longer necessary, but it would still be valuable, as it
would let us directly emit better IR. It shouldn't be part of this
change, though.