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

Alexey Samsonov vonosmas at gmail.com
Wed Jun 4 17:08:15 PDT 2014


On Tue, Jun 3, 2014 at 12:33 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Jun 1, 2014, at 11:23 AM, Alexey Samsonov <vonosmas at gmail.com> wrote:
>
>
> On Fri, May 30, 2014 at 7:14 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> > On May 30, 2014, at 4:55 PM, Alexey Samsonov <vonosmas at gmail.com>
>> wrote:
>> >
>> >
>> > On Fri, 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.
>> >
>> > Could you elaborate on line table solution?
>>
>> Epilogue is everything that is dominated by the last instruction that has
>> a DebugLoc attached to it. This incidentally also happens to be the
>> debugger’s interpretation.
>>
>
> But this is not true - I see that every instruction in the epilogue
> (including the ones that restore stack and frame pointer, and the return
> instruction itself) have the valid DebugLoc attached to them.
>
>
> You're right. We’d have to scan back from the last instruction to find the
> first instruction that has a different DebugLoc. While not an epitome of
> elegance, this seems to work fine:
>

SG. I've implemented this suggestion and uploaded the new version of patch.


>
> /// Return an iterator pointing to the first epilogue instruction in
> /// MBB by looking at the instructions' debug location. Returns
> /// MBB->end() if there are no epilogue instructions.
> static MachineBasicBlock::const_iterator
> getFirstEpilogueInstr(const MachineBasicBlock &MBB) {
>   const auto Last = MBB.getLastNonDebugInstr();
>   bool isReturnBlock = MBB.succ_empty();
>

^^
I think we should check that Last is return instruction here. We can have
unreachable basic blocks,
or basic blocks ending with noreturn calls.


>   if (!isReturnBlock || Last == MBB.end())
>     return MBB.end();
>
>   DebugLoc LastLoc = Last->getDebugLoc();
>   MachineBasicBlock::const_reverse_iterator First(Last);
>   for (auto MI = First; MI != MBB.instr_rend(); ++MI) {
>     if (MI->getDebugLoc() != LastLoc)
>       return MachineBasicBlock::const_iterator(&*First);
>     First = MI;
>   }
>   return MBB.begin();
> }
>
> void calculateDbgValueHistory(const MachineFunction *MF,
>                               const TargetRegisterInfo *TRI,
>                               DbgValueHistoryMap &Result) {
>   // Collect registers that are modified in the function body (their
> contents
>   // is changed after the frame setup and before the last basic block).
>   std::set<unsigned> ChangingRegs;
>   for (const auto &MBB : *MF) {
>     for (auto MI = MBB.instr_begin(); MI != getFirstEpilogueInstr(MBB);
> ++MI) {
>       if (!MI->getFlag(MachineInstr::FrameSetup) && &MBB != &MF->back())
>         collectClobberedRegisters(*MI, TRI, ChangingRegs);
>     }
>   }
>     ...
>
> cheers,
> Adrian
>
>
>
>>
>> >
>> >
>> > -- 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
>> > >
>> >
>> >
>> >
>> >
>> > --
>> > Alexey Samsonov
>> > vonosmas at gmail.com
>>
>>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com
>
>
>


-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140604/018258ad/attachment.html>


More information about the llvm-commits mailing list