[llvm] r228463 - [AArch64] Use the source location of the IR branch when creating Bcc

[Ahmed Bougacha]

On Fri, Feb 6, 2015 at 3:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Fri, Feb 6, 2015 at 3:36 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
>>
>> On Fri, Feb 6, 2015 at 3:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Fri, Feb 6, 2015 at 3:15 PM, Ahmed Bougacha
>> > <ahmed.bougacha at gmail.com>
>> > wrote:
>> >>
>> >> Author: ab
>> >> Date: Fri Feb  6 17:15:39 2015
>> >> New Revision: 228463
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=228463&view=rev
>> >> Log:
>> >> [AArch64] Use the source location of the IR branch when creating Bcc
>> >> from a conditional branch fed by an add/sub/mul-with-overflow node.
>> >>
>> >> We previously used the SDLoc of the overflow node, for no good reason.
>> >> In some cases, this led to the Bcc and B terminators having different
>> >> source orders, and DBG_VALUEs being inserted between them.
>> >>
>> >> The real issue is with the code that can't handle DBG_VALUEs between
>> >> terminators: the few places affected by this will be fixed soon.
>> >> In the meantime, fixing the SDLoc is a positive change no matter what.
>> >>
>> >> No tests, as I have no idea how to get .loc emitted for branches?
>> >
>> >
>> > .loc should be emitted for any instruction with a debug location (clang
>> > does
>> > omit debug locations on various branches from the frontend, but you can
>> > add
>> > them in by hand or find a case where clang doesn't omit them... ).
>>
>> I did tweak the !dbg metadata for the branch (see attached testcase),
>> but I only get:
>>
>>         .loc    1 21 21 is_stmt 1       ; /path:21:21
>>         str      ...
>>         b.vs    ...
>>
>> That is, the instruction before the branch (the store) gets a .loc,
>> but not the branch itself.
>> Am I missing something?
>
>
> Possibly a bug elsewhere in debug location preservation - there are a few
> down tehre in the backend for example where two instructions are coalesced
> into one MachineInstr (thus having only one location) then split out again,
> (where they both get given the same location), etc.

I figured as much.. filed http://llvm.org/bugs/show_bug.cgi?id=22498
I'll have a look if no one beats me to it.

Thanks!

-Ahmed

>>
>>
>> -Ahmed
>>
>> >>
>> >>
>> >> rdar://19347133
>> >>
>> >> Modified:
>> >>     llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>> >>
>> >> Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=228463&r1=228462&r2=228463&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp (original)
>> >> +++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp Fri Feb  6
>> >> 17:15:39 2015
>> >> @@ -3263,8 +3263,8 @@ SDValue AArch64TargetLowering::LowerBR_C
>> >>        OFCC = getInvertedCondCode(OFCC);
>> >>      SDValue CCVal = DAG.getConstant(OFCC, MVT::i32);
>> >>
>> >> -    return DAG.getNode(AArch64ISD::BRCOND, SDLoc(LHS), MVT::Other,
>> >> Chain,
>> >> Dest,
>> >> -                       CCVal, Overflow);
>> >> +    return DAG.getNode(AArch64ISD::BRCOND, dl, MVT::Other, Chain,
>> >> Dest,
>> >> CCVal,
>> >> +                       Overflow);
>> >>    }
>> >>
>> >>    if (LHS.getValueType().isInteger()) {
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>
>