[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Eric Christopher]

FWIW this is on my list. A quick glance sounded reasonable, I've not looked
at it in detail though.

Thanks :)

-eric

On Mon Jan 12 2015 at 12:51:09 PM Ahmed Bougacha <ahmed.bougacha at gmail.com>
wrote:

> Ping for opinions!
>
> Thanks,
> - Ahmed
>
>
> On Wed, Jan 7, 2015 at 4:06 AM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
> > W.r.t. trunk not crashing on the testcase: I bisected it down to
> > r221973 making the assertion not fire in some cases where it used to.
> > I went ahead and changed the assertion in r225338.  Now the attached
> > testcase crashes, as it should.
> >
> > - Ahmed
> >
> > On Tue, Jan 6, 2015 at 2:49 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
> >> Hi all,
> >>
> >> In some situations (see below for testcase explanation), we were
> >> crashing in various places, because of a single root problem: a
> >> MachineBasicBlock with DBG_VALUEs between the two terminators (a
> >> conditional and an unconditional branch).
> >>
> >> It all goes downhill from there, with at least 4 different places
> >> where this breaks code assuming that the terminator before-last is
> >> immediately preceding the last one.
> >>
> >> Now, if the DBG_VALUEs weren't there in the first place, we wouldn't
> >> have this problem.  However, most code already accepts DBG_VALUEs at
> >> the end of a block, after the last terminator.  My understanding is,
> >> it's not ideal nor well-tested, but we can have them between
> >> terminators as well.  If not, we should make it explicit.
> >>
> >> Attached are 5 small patches, the most important being:
> >>
> >>     [CodeGen] Add a MBB::getPreviousTerminator method, ignoring
> DBG_VALUEs.
> >>       ...
> >>     This commit adds a similar method, that returns an iterator to the
> >>     last terminator preceding the instruction passed as an iterator.
> >>
> >> The others fix various uses in EarlyIfCvt, MBB, and AArch64, all
> >> breaking on my local testcase:
> >>
> >>     [CodeGen] Ignore DBG_VALUEs between terminators when replacing MBB
> uses.
> >>     [CodeGen][EarlyIfCvt] Ignore DBG_VALUEs between terminators.
> >>     [AArch64] Ignore DBG_VALUEs in AnalyzeBranch, by using
> >> getPreviousTerminator.
> >>     [AArch64] Ignore DBG_VALUEs in RemoveBranch, by using
> getPreviousTerminator.
> >>
> >> These fix the testcase, but I counted at least 5 other suspicious uses
> >> of isTerminator.
> >> Also, there are a few innocuous loops that can be replaced by
> >> getPreviousTerminator.
> >> If the general idea is sound, I'll fix those as well.
> >>
> >> I'm not sure how to write tests that wouldn't be extremely brittle;
> >> ideas welcome.  The closest I have is my local testcase (attached,
> >> reduced from webkit), breaking some older revisions of clang when
> >> targeting AArch64.  Because of seemingly unrelated changes (I'm
> >> looking into it), the crashes don't happen anymore on trunk, but the
> >> problems are still there.
> >>
> >> Thanks!
> >>
> >> - Ahmed
> _______________________________________________
> 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/20150112/74b160ba/attachment.html>