[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Ahmed Bougacha]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-CodeGen-Add-a-MBB-getPreviousTerminator-method-ignor.patch
Type: application/octet-stream
Size: 2911 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/699bfe97/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-CodeGen-Ignore-DBG_VALUEs-between-terminators-when-r.patch
Type: application/octet-stream
Size: 934 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/699bfe97/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-CodeGen-EarlyIfCvt-Ignore-DBG_VALUEs-between-termina.patch
Type: application/octet-stream
Size: 1164 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/699bfe97/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-AArch64-Ignore-DBG_VALUEs-in-AnalyzeBranch-by-using-.patch
Type: application/octet-stream
Size: 2453 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/699bfe97/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-AArch64-Ignore-DBG_VALUEs-in-RemoveBranch-by-using-g.patch
Type: application/octet-stream
Size: 875 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/699bfe97/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbgvalue_terminators_testcase.ll
Type: application/octet-stream
Size: 39647 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/699bfe97/attachment-0005.obj>