[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Ahmed Bougacha]

Gentle ping.

-Ahmed

On Mon, Jan 12, 2015 at 2:14 PM, Eric Christopher <echristo at gmail.com> wrote:
> FWIW this is on my list. A quick glance sounded reasonable, I've not looked
> at it in detail though.

Thanks Eric!

> 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