[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Ahmed Bougacha]

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