[llvm] r227481 - DebugInfo: Teach Fast ISel to respect the debug location of comparisons in jumps

[Eric Christopher]

On Thu Jan 29 2015 at 11:50:19 AM David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Jan 29, 2015 at 11:20 AM, Eric Christopher <echristo at gmail.com>
> wrote:
>
>> Agreed.
>>
>> Also, testcase?
>>
>
> Ah, thanks for the catch - forgot to git add. Committed it r227485.
>
>
>> And the other fast-isel ports? :)
>>
>
> No reason to believe the other ports have the same bug, though I haven't
> looked (I always get confused by the triple/target syntax, to target
> another architecture (circular problem - don't do it often enough to be
> familiar and avoid doing it due to unfamiliarity)). A quick search of
> similar functions (such as 'FastEmitCompare') doesn't turn up any results
> in the other FastISel implementations... I see, different naming
> conventions, emitCmp, SelectCmp, EmitCmp...
>
> But yeah, generally suffer from the same bug. (though I'm not sure how
> much value there is in fixing up this one case given the breadth of the
> issue here - removing DbgLoc and changing all uses to explicitly handle
> DebugLocs from Instruction to MachineInstr is going to be necessary to make
> this stuff better - though I don't plan on doing that personally any time
> soon)
>
> I'll poke at the other targets, see if there's a reasonable test case
> generalization (maybe if I'm lucky they'll all have 'cmp' somewhere in the
> instruction name? ;)).
>
>

Yeah, sorry, as we were writing them we changed naming conventions while
doing it. :\

-eric


>
>> -eric
>>
>> On Thu Jan 29 2015 at 11:16:03 AM David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>> Author: dblaikie
>>> Date: Thu Jan 29 13:09:18 2015
>>> New Revision: 227481
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=227481&view=rev
>>> Log:
>>> DebugInfo: Teach Fast ISel to respect the debug location of comparisons
>>> in jumps
>>>
>>> The use of the DbgLoc in FastISel is probably something we should fix.
>>> It's prone to leaking the wrong location into instructions - we should
>>> have a clear chain of custody from the debug location of an IR
>>> Instruction to that of a MachineInstr to avoid such leakage.
>>>
>>> Modified:
>>>     llvm/trunk/lib/Target/X86/X86FastISel.cpp
>>>
>>> Modified: llvm/trunk/lib/Target/X86/X86FastISel.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/
>>> X86/X86FastISel.cpp?rev=227481&r1=227480&r2=227481&view=diff
>>> ============================================================
>>> ==================
>>> --- llvm/trunk/lib/Target/X86/X86FastISel.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/X86FastISel.cpp Thu Jan 29 13:09:18 2015
>>> @@ -80,7 +80,7 @@ public:
>>>  #include "X86GenFastISel.inc"
>>>
>>>  private:
>>> -  bool X86FastEmitCompare(const Value *LHS, const Value *RHS, EVT VT);
>>> +  bool X86FastEmitCompare(const Value *LHS, const Value *RHS, EVT VT,
>>> DebugLoc DL);
>>>
>>>    bool X86FastEmitLoad(EVT VT, const X86AddressMode &AM,
>>> MachineMemOperand *MMO,
>>>                         unsigned &ResultReg);
>>> @@ -1109,7 +1109,7 @@ static unsigned X86ChooseCmpImmediateOpc
>>>  }
>>>
>>>  bool X86FastISel::X86FastEmitCompare(const Value *Op0, const Value
>>> *Op1,
>>> -                                     EVT VT) {
>>> +                                     EVT VT, DebugLoc CurDbgLoc) {
>>>    unsigned Op0Reg = getRegForValue(Op0);
>>>    if (Op0Reg == 0) return false;
>>>
>>> @@ -1122,7 +1122,7 @@ bool X86FastISel::X86FastEmitCompare(con
>>>    // CMPri, otherwise use CMPrr.
>>>    if (const ConstantInt *Op1C = dyn_cast<ConstantInt>(Op1)) {
>>>      if (unsigned CompareImmOpc = X86ChooseCmpImmediateOpcode(VT,
>>> Op1C)) {
>>> -      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
>>> TII.get(CompareImmOpc))
>>> +      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, CurDbgLoc,
>>> TII.get(CompareImmOpc))
>>>          .addReg(Op0Reg)
>>>          .addImm(Op1C->getSExtValue());
>>>        return true;
>>> @@ -1134,7 +1134,7 @@ bool X86FastISel::X86FastEmitCompare(con
>>>
>>>    unsigned Op1Reg = getRegForValue(Op1);
>>>    if (Op1Reg == 0) return false;
>>> -  BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(CompareOpc))
>>> +  BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, CurDbgLoc,
>>> TII.get(CompareOpc))
>>>      .addReg(Op0Reg)
>>>      .addReg(Op1Reg);
>>>
>>> @@ -1202,7 +1202,7 @@ bool X86FastISel::X86SelectCmp(const Ins
>>>
>>>    ResultReg = createResultReg(&X86::GR8RegClass);
>>>    if (SETFOpc) {
>>> -    if (!X86FastEmitCompare(LHS, RHS, VT))
>>> +    if (!X86FastEmitCompare(LHS, RHS, VT, I->getDebugLoc()))
>>>        return false;
>>>
>>>      unsigned FlagReg1 = createResultReg(&X86::GR8RegClass);
>>> @@ -1227,7 +1227,7 @@ bool X86FastISel::X86SelectCmp(const Ins
>>>      std::swap(LHS, RHS);
>>>
>>>    // Emit a compare of LHS/RHS.
>>> -  if (!X86FastEmitCompare(LHS, RHS, VT))
>>> +  if (!X86FastEmitCompare(LHS, RHS, VT, I->getDebugLoc()))
>>>      return false;
>>>
>>>    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc),
>>> ResultReg);
>>> @@ -1353,7 +1353,7 @@ bool X86FastISel::X86SelectBranch(const
>>>          std::swap(CmpLHS, CmpRHS);
>>>
>>>        // Emit a compare of the LHS and RHS, setting the flags.
>>> -      if (!X86FastEmitCompare(CmpLHS, CmpRHS, VT))
>>> +      if (!X86FastEmitCompare(CmpLHS, CmpRHS, VT, CI->getDebugLoc()))
>>>          return false;
>>>
>>>        BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
>>> TII.get(BranchOpc))
>>> @@ -1740,7 +1740,7 @@ bool X86FastISel::X86FastEmitCMoveSelect
>>>
>>>      EVT CmpVT = TLI.getValueType(CmpLHS->getType());
>>>      // Emit a compare of the LHS and RHS, setting the flags.
>>> -    if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT))
>>> +    if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT, CI->getDebugLoc()))
>>>        return false;
>>>
>>>      if (SETFOpc) {
>>> @@ -1924,7 +1924,7 @@ bool X86FastISel::X86FastEmitPseudoSelec
>>>        std::swap(CmpLHS, CmpRHS);
>>>
>>>      EVT CmpVT = TLI.getValueType(CmpLHS->getType());
>>> -    if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT))
>>> +    if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT, CI->getDebugLoc()))
>>>        return false;
>>>    } else {
>>>      unsigned CondReg = getRegForValue(Cond);
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150129/9d635a55/attachment.html>