[PATCH][CodeGen] Ignoring DBG_VALUEs between terminators?

[Ahmed Bougacha]

On Fri, Feb 6, 2015 at 2:11 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> 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

And this is fixed in r228463.

-Ahmed

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