[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Ahmed Bougacha]

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