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

[Alexey Samsonov]

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.


>
> >
> >
> > -- 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140601/3fd63a67/attachment.html>