r224329 - Renamed RefersToEnclosingLocal bitfield to RefersToCapturedVariable.

[Bataev, Alexey]

Hi Richard, John,
Done in r225624.

Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team

06.01.2015 8:45, John McCall пишет:
>> On Jan 5, 2015, at 7:05 AM, Richard Smith <richard at metafoo.co.uk 
>> <mailto:richard at metafoo.co.uk>> wrote:
>> On Sun, Jan 4, 2015 at 7:57 PM, John McCall <rjmccall at apple.com 
>> <mailto:rjmccall at apple.com>> wrote:
>>
>>>     On Jan 4, 2015, at 6:28 PM, Richard Smith <richard at metafoo.co.uk
>>>     <mailto:richard at metafoo.co.uk>> wrote:
>>>
>>>     On 5 Jan 2015 02:08, "John McCall" <rjmccall at apple.com
>>>     <mailto:rjmccall at apple.com>> wrote:
>>>     >>
>>>     >> On Jan 4, 2015, at 1:11 AM, Richard Smith
>>>     <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>>     >> On Sat, Jan 3, 2015 at 1:46 PM, John McCall
>>>     <rjmccall at apple.com <mailto:rjmccall at apple.com>> wrote:
>>>     >>>>
>>>     >>>> On Jan 2, 2015, at 7:10 AM, Richard Smith
>>>     <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>>     >>>> On Tue, Dec 16, 2014 at 12:01 AM, Alexey Bataev
>>>     <a.bataev at hotmail.com <mailto:a.bataev at hotmail.com>> wrote:
>>>     >>>>>
>>>     >>>>> Author: abataev
>>>     >>>>> Date: Tue Dec 16 02:01:48 2014
>>>     >>>>> New Revision: 224329
>>>     >>>>>
>>>     >>>>> URL: http://llvm.org/viewvc/llvm-project?rev=224329&view=rev
>>>     >>>>> Log:
>>>     >>>>> Renamed RefersToEnclosingLocal bitfield to
>>>     RefersToCapturedVariable.
>>>     >>>>> Bitfield RefersToEnclosingLocal of
>>>     Stmt::DeclRefExprBitfields renamed to RefersToCapturedVariable
>>>     to reflect latest changes introduced in commit 224323. Also
>>>     renamed method Expr::refersToEnclosingLocal() to
>>>     Expr::refersToCapturedVariable() and comments for constant
>>>     arguments.
>>>     >>>>> No functional changes.
>>>     >>>>
>>>     >>>>
>>>     >>>> This seems like a bad idea for me. It's incorrect: for a
>>>     lambda, the flag means that the DeclRefExpr refers to an
>>>     enclosing local, and does *not* imply that the variable is
>>>     necessarily captured. This confusion has already led to a bug
>>>     (fixed in r225060).
>>>     >>>
>>>     >>>
>>>     >>> If that’s actually a useful property to track, I have no
>>>     complaint about tracking the two things separately.  I don’t
>>>     think DRE is short of bits.
>>>     >>
>>>     >>
>>>     >> The problem is that we can't reasonably track whether a DRE
>>>     refers to a captured variable, because that is not a local
>>>     property of the DRE; it depends on non-trivial properties of the
>>>     context in which the DRE is used (and more generally it depends
>>>     on how else that same variable is used elsewhere in the lambda,
>>>     but we can largely ignore that).
>>>     >>
>>>     >> I'd be fine having separate flags for 'refers to enclosing
>>>     local' and 'refers to captured global' (if the latter is the
>>>     point of this change), but we should not claim a DRE refers to a
>>>     capture when it does not.
>>>     >
>>>     >
>>>     > Richard, I understand that not all DREs are potentially
>>>     evaluated and thus may not actually induce capturing, and I
>>>     concede your point that the flag's name shouldn’t use the word
>>>     “capture” if it’s set on unevaluated DREs.  The question is
>>>     this: what information do we actually need to track on DREs?
>>>     >
>>>     > It is very useful for IR-gen to be able to immediately know
>>>     whether a DRE refers to a captured local or global variable, no
>>>     matter why that capture was performed.  As far as I’m aware,
>>>     IR-gen does not care about whether the flag is set on
>>>     unevaluated references to enclosing variables, because IR-gen
>>>     should never see an unevaluated use in normal expression emission.
>>>
>>>     This is not about unevaluated references, it's about evaluated
>>>     references that aren't odr uses, which depends on the context in
>>>     which the DRE appears. IRGen does need to deal with these cases.
>>>
>>     Ugh, fine, I will go haul out the standard to figure out what
>>     you’re talking about, since you are apparently in a vague mood
>>     tonight.
>>
>>
>> Apologies for the terseness, was replying from a phone.
>>
>>     Okay, I believe what you’re saying is:
>>     1. A reference to an entity (a variable/this) within a lambda
>>     only causes a capture if it's an ODR use.
>>     2. All ODR uses are PE references, but a PE reference isn't an
>>     ODR use if the variable can be used in a constant expression and
>>     it's a potential result of an expression that’s either ignored or
>>     loaded.
>>     3. Whether an expression is ignored or loaded cannot be decided
>>     immediately from context; you have to semantically process the
>>     entire expression, and it may be different in different template
>>     instantiations.  (This dependence is also true of PPE-ness, I think.)
>>     4. Since we sometimes share DRE nodes, we can’t retroactively
>>     alter a node after doing the necessary semantic analysis.
>>     5. AST clients are likely to want to know whether a DRE is not an
>>     ODR-use and can only be constant-evaluated.  For example, it’s
>>     important that IR-gen not actually emit a reference to a static
>>     member variable of a class template if that member variable
>>     hasn’t been ODR-used.  Clients should similarly not be confused
>>     by an evaluated reference to an uncaptured enclosing local.
>>
>>     I think point #4 tells us that the desired information in #5
>>     can’t actually be directly represented in the AST, though.
>>
>>
>> Yes, that's an excellent summary. For concreteness, here's an example 
>> where this happens:
>> void f(int, int);
>> void f(const int&, double);
>> auto f() {
>>    const int n = 0;
>>    return [=](auto t) { return [=]() { f(n, t); } };
>> }
>> void g() { f()(0)(); f()(0.)(); }
>> Here, the lambda returned by f()(0) does not capture 'n', but the 
>> lambda returned by f()(0.) does, and they share a DeclRefExpr for n.
>>
>>     But as the first example shows, client interests in #5 are much
>>     broader than just references to enclosing locals.  IR-gen would
>>     benefit from knowing whether an arbitrary expression was
>>     constant-evaluable; currently it has hacky code to try to
>>     constant-evaluate specific nodes, and it lets all the other nodes
>>     fall out from LLVM’s own constant-folding and optimization, but
>>     it would clearly be better to, say, know that a particular call
>>     is a call to a constexpr function with constant arguments and so
>>     we can just directly try to fold it instead of emitting a whole
>>     bunch of unnecessary code that will get cleaned up later.  Using
>>     “this is an enclosing local” as an incomplete approximation of
>>     that isn’t actually useful.
>>
>>
>> The only cases where we need to emit different code based on whether 
>> an expression is a constant expression (outside unevaluated contexts) 
>> are when initializing a variable of static/thread_local storage 
>> duration, and when an expression refers to an enclosing local 
>> variable that it doesn't capture.
>
> You’re forgetting references to static data members, which don’t have 
> to be defined if they have a constant initializer in the class 
> definition and are never ODR-used.  (Of course, in practice people 
> learn to avoid writing code that relies on this because it’s too easy 
> to accidentally ODR-use a variable; but it’s in the spec.)  This is 
> something that can’t as easily be hacked around in IRGen.
>
>> In all other cases, it's correct and often reasonable to evaluate the 
>> expression at runtime.
>
> It’s correct, but it’s easy to imagine code where this causes a great 
> deal of unnecessary code to be emitted, optimized, and finally thrown 
> away.
>
> Also we probably have bugs involving some of the recursive cases of 
> potential results where IRGen will introduce illegal symbol 
> references.  I’m particularly concerned about loads from members.
>
>> In the case where a lambda expression refers to an enclosing local 
>> but doesn't capture it, we should emit a copy of that local's value 
>> as a constant global.
>
> Sure.
>
>>     So it seems to me that we just need one bit, called
>>     refersToEnclosingVariableOrCapture(), with the meaning that it’s
>>     set if the DRE refers to an enclosing local (captured or not) or
>>     captured global variable, regardless of whether this reference is
>>     an ODR use.
>>
>>
>> That seems fine to me.
>
> Okay.  Alexey, would you mind doing this after you get back from break?
>
> John.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150112/67345c76/attachment.html>