On Thu, Jul 11, 2013 at 4:44 PM, Richard Smith <[email protected]> wrote: > On Thu, Jul 11, 2013 at 12:37 PM, Faisal Vali <[email protected]> wrote: >> On Wed, Jul 10, 2013 at 6:57 PM, John McCall <[email protected]> wrote: >>> On Jul 10, 2013, at 4:51 PM, Faisal Vali <[email protected]> wrote: >>>> On Wed, Jul 10, 2013 at 4:56 PM, Richard Smith <[email protected]> >>>> wrote: >>>>> On Wed, Jul 10, 2013 at 10:06 AM, Faisal Vali <[email protected]> wrote: >>>>>> On Wed, Jun 12, 2013 at 7:02 PM, Richard Smith <[email protected]> >>>>>> wrote: >>>>>>> On Sun, Jun 9, 2013 at 12:47 PM, Faisal Vali <[email protected]> wrote: >>>>>>>> >>>>>>>> On Fri, Jun 7, 2013 at 5:21 PM, Richard Smith <[email protected]> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> On Tue, Jun 4, 2013 at 9:19 PM, Faisal Vali <[email protected]> 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!
DR712--submit-make-static-cleaner.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
