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

Adrian Prantl aprantl at apple.com
Fri May 30 19:17:19 PDT 2014 

> On May 30, 2014, at 4:32 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 
> 
> 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.

Yes, I think instead of the last basic block we should be talking about basic blocks that end with a return instruction.

-- adrian

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

More information about the llvm-commits mailing list