[PATCH] Generate better location ranges for some register-described variables.

[Adrian Prantl]

> On May 30, 2014, at 4:13 PM, Adrian Prantl <aprantl at apple.com> wrote:
> 
> 
>> On May 30, 2014, at 3:57 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>> 
>> 
>> On Fri, May 30, 2014 at 3:54 PM, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>>> On May 30, 2014, at 3:51 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>>> 
>>> 
>>> On Fri, May 30, 2014 at 3:44 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> 
>>>> On May 30, 2014, at 3:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> 
>>>> On Fri, May 30, 2014 at 3:37 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>> 
>>>>>> On May 30, 2014, at 3:31 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>> 
>>>>>> On Fri, May 30, 2014 at 3:12 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>>>>> On Fri, May 30, 2014 at 3:03 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>>> On Fri, May 30, 2014 at 3:00 PM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>>>>>>>>> 
>>>>>>>>> On Fri, May 30, 2014 at 12:45 PM, Eric Christopher <echristo at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>> On Fri, May 30, 2014 at 10:26 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> On May 29, 2014, at 10:03 PM, Alexey Samsonov <vonosmas at gmail.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> Yeah, well, we can add "FrameTeardown" machine instruction flag in
>>>>>>>>>>>> addition to "FrameSetup", and annotate machine instructions added in frame
>>>>>>>>>>>> lowering code. But do we expect many users of this flag (assuming that this
>>>>>>>>>>>> usage is also somewhat questionable)?
>>>>>>>>>>> 
>>>>>>>>>>> The FrameSetup flag is also only used by the debug info, so at least it
>>>>>>>>>>> wouldn’t be any more questionable than that :-)
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> OK. Why don't we land the patch as-is now (don't terminate the register at
>>>>>>>>> the end of MBB if this register is only modified in frame setup and the last
>>>>>>>>> MBB - it should be safe), and then replace "the last MBB" condition with
>>>>>>>>> checking a "FrameTeardown" flag, attached to certain machine instructions?
>>>>>>>>> In this way we won't have to special-case certain registers and make
>>>>>>>>> assumptions about their constant-ness.
>>>>>>>> 
>>>>>>>> Given the limited nature of this fix (both in the fix's possible scope
>>>>>>>> and intent/purpose) I'm personally sort of inclined to do this by
>>>>>>>> whitelisting certain registers instead, if that were/is possible...
>>>>>>>> seems simpler and still sufficient for the required purpose here.
>>>>>>>> 
>>>>>>>> Can we easily just test whether a register is the stack/frame pointer?
>>>>>>>> (I actually know so little about this that the difference between
>>>>>>>> those two things isn't straight in my head and I'm not sure which, or
>>>>>>>> both, we need for the common/intended case here)
>>>>>>>> 
>>>>>>> 
>>>>>>> If we need to stack realign we might not be able to do this as easily.
>>>>>>> I'm not a huge fan of either method, but I think that the assumption
>>>>>>> that Alexey has might make more sense than whitelisting.
>>>>>> 
>>>>>> Fair enough.
>>>>>> 
>>>>>>> My logic:
>>>>>>> Whitelisting involves assuming that various registers won't be used in
>>>>>>> ways that surprise us and I can think of a few ways where it could
>>>>>>> happen: stack realignment, inline asm, a smarter stack coloring
>>>>>>> algorithm.
>>>>>> 
>>>>>> Can inline asm clobber the frame pointer & we actually tolerate that,
>>>>>> find the right variables, etc, when that happens? (how?)
>>>>>> 
>>>>>> Can't quite picture the stack coloring you have in mind & Alexey
>>>>>> commented on the stack realignment issue.
>>>>>> 
>>>>>> The only other idea I'd float would be another simpler solution,
>>>>>> though a bit more expressive: just a check to see that the particular
>>>>>> register(s) (stack/frame pointers, whichever things it is we actually
>>>>>> use to describe the location of stack variables) don't change beyond
>>>>>> the prologue, and if not, describe any variables that are relative to
>>>>>> that register as having that location for the whole program.
>>>>> 
>>>>> That won’t work for values that are spilled to the stack, because they only live on the stack for part of the function.
>>>> 
>>>> Do we get those right currently?
>>> 
>>> Partially. We get them right until the end of the current basic block.
>>> 
>>> The patch should improve this situation by the way:
>>> <bbX>:
>>>  DBG_VALUE $rsp, 32 !"var"  // "var is spilled on stack.
>>> // ... more code ..
>>> <bbY>:
>>>  DBG_VALUE $r9, 0, !"var"  // "var" is now loaded to "r9".
>>> 
>>> Earlier we used to terminate the location of "var" at the end of bbX, but now we won't do
>>> this, as we know "rsp" doesn't change in the function.
>> 
>> Yeah, but only if we can detect the Epilogue and ignore the def of $rsp that restores the register at the end of the function.
>> 
>> Yes. So, how do you feel about the plan suggested above:
>> 
>> OK. Why don't we land the patch as-is now (don't terminate the register at
>> the end of MBB if this register is only modified in frame setup and the last
>> MBB - it should be safe), and then replace "the last MBB" condition with
>> checking a "FrameTeardown" flag, attached to certain machine instructions?
>> In this way we won't have to special-case certain registers and make
>> assumptions about their constant-ness.
> 
> Instead of checking for it being the last basic block we could check for something like (BB.getLastNonDebugInstr()->isReturn()) then we would handle cases with multiple return blocks. The only problem that I see is that the last basic block may also contain non-epilogue code. In fact, the entire function may only be a single basic block. A safer condition might be “register is modified in the prologue && has precisely one non-prologue-def in each returning basic block”?
> Then again that condition would break on a backend that pops each callee-saved register from the stack one-by-one.
> 
> What about this definition: The epilogue is everything after the last instruction that has a DebugLoc attached to it?

Sorry, I realized didn’t actually answer you question :-)
I agree that this is a big improvement, but I’m not convinced that the condition for detecting the epilogue you mention is safe, for the same reasons that Eric mentioned earlier. When we find a safe condition for the epilogue this is good to go. My current favorite is looking at the line table.

-- adrian

> 
> -- adrian
>> 
>> 
>>> 
>>> But there is no code that would insert DBG_VALUES after they are reloaded again.
>>> 
>>> 
>>>> 
>>>>> [And for (-O0) allocas that are described in the MMI table, we already do what you described].
>>>> 
>>>> Right - but the problem Alexey is trying to address is what ASan does,
>>>> where it makes one big alloca and describes variables as being within
>>>> that alloca - so the MMI sidetable handling doesn't fire…
>>> 
>>> Awright.
>>> 
>>> Yeah, MMI side table doesn't really work for indirect descriptions, when we need to deref the memory stored at reg+offset.
>>> 
>>> 
>>>> 
>>>>> 
>>>>> -- adrian
>>>>> 
>>>>>> 
>>>>>> But, yeah, that's not /lots/ easier than finding any registers that
>>>>>> happen to meet that property.
>>>>>> 
>>>>>> - David
>>>>>> 
>>>>>>> 
>>>>>>> -eric
>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> :)
>>>>>>>>>> 
>>>>>>>>>>>> I can also make this patch even less general and just special-case the
>>>>>>>>>>>> frame pointer and the stack pointer registers, so that the code wouldn't
>>>>>>>>>>>> pretend to solve a general problem we're facing.
>>>>>>>>>>> 
>>>>>>>>>>> The frame pointer should be stable over the entire function, special
>>>>>>>>>>> casing it seems to be appropriate. I don’t know whether, e.g, stack coloring
>>>>>>>>>>> would cause the stack pointer to be modified in the middle of a function.
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> FWIW it may not _now_ but there's nothing from stopping it either. :\
>>>>>>>>>> 
>>>>>>>>>> -eric
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> --
>>>>>>>>> Alexey Samsonov
>>>>>>>>> vonosmas at gmail.com
>>> 
>>> 
>>> 
>>> 
>>> --
>>> Alexey Samsonov
>>> vonosmas at gmail.com
>> 
>> 
>> 
>> 
>> -- 
>> Alexey Samsonov
>> vonosmas at gmail.com
>