[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Ahmed Bougacha]

On Mon, Jan 19, 2015 at 5:47 PM, Matthias Braun <mbraun at apple.com> wrote:
> 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...

Well, I can understand why it happens: the DBG_VALUE MIs are inserted
according to IR source order.  On AArch64, when lowering BR_CC, in
some cases (an overflow intrinsic feeding the condition) the SDLoc
used to create the AArch64::Bcc is not the one used for the
AArch64::B..

There's no good reason to do that here (actually let me fix that right
away), but I can imagine creating a conditional branch from another
node, that has a different source order from the last terminator.
So yeah, not sure which way to go.

(Also, ping!)

-Ahmed

> - 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
>