[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?
echristo at gmail.com
Mon Jan 12 14:14:50 PST 2015
FWIW this is on my list. A quick glance sounded reasonable, I've not looked
at it in detail though.
On Mon Jan 12 2015 at 12:51:09 PM Ahmed Bougacha <ahmed.bougacha at gmail.com>
> Ping for opinions!
> - Ahmed
> On Wed, Jan 7, 2015 at 4:06 AM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> > 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>
> >> 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
> >> ...
> >> 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
> >> [CodeGen][EarlyIfCvt] Ignore DBG_VALUEs between terminators.
> >> [AArch64] Ignore DBG_VALUEs in AnalyzeBranch, by using
> >> getPreviousTerminator.
> >> [AArch64] Ignore DBG_VALUEs in RemoveBranch, by using
> >> 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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits