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

Jim Grosbach grosbach at apple.com
Fri May 30 16:32:07 PDT 2014 

On May 30, 2014, at 4:17 PM, Adrian Prantl <aprantl at apple.com> wrote:

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

Note that tail duplication can, and often does, fire for epilogue code. The epilogue is often neither only in one place nor the last basic block in a function.

-Jim


> -- 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
>> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140530/873ff6f0/attachment.html>

More information about the llvm-commits mailing list