[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Matthias Braun]

Just looking at the MachineBasicBlock::terminators() API would imply to me that there is an iterator range that contains all terminators and nothing else. Also a basic block is usually defined as having the terminator at the end of the basic block (I haven't seen a definition with multiple terminators yet though). If there are no good reasons for having DBG_VALUEs between or after terminators I'd disallow them there and enforce the rule with the MachineVerifier...

- Matthias

> On Jan 19, 2015, at 4:53 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> 
> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits