<div dir="ltr"><br><br><div class="gmail_quote">On Thu Jan 29 2015 at 11:50:19 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 29, 2015 at 11:20 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Agreed.<br><br>Also, testcase? </div></blockquote><div><br>Ah, thanks for the catch - forgot to git add. Committed it r227485.<br> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">And the other fast-isel ports? :)</div></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>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... <br><br>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)<br><br>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? ;)).<br> </div></div></div></div></blockquote><div><br></div><div>Yeah, sorry, as we were writing them we changed naming conventions while doing it. :\</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>-eric</div></font></span></div><div><div><br><div class="gmail_quote">On Thu Jan 29 2015 at 11:16:03 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: dblaikie<br>
Date: Thu Jan 29 13:09:18 2015<br>
New Revision: 227481<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=227481&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=227481&view=rev</a><br>
Log:<br>
DebugInfo: Teach Fast ISel to respect the debug location of comparisons in jumps<br>
<br>
The use of the DbgLoc in FastISel is probably something we should fix.<br>
It's prone to leaking the wrong location into instructions - we should<br>
have a clear chain of custody from the debug location of an IR<br>
Instruction to that of a MachineInstr to avoid such leakage.<br>
<br>
Modified:<br>
llvm/trunk/lib/Target/X86/<u></u>X86FastISel.cpp<br>
<br>
Modified: llvm/trunk/lib/Target/X86/<u></u>X86FastISel.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FastISel.cpp?rev=227481&r1=227480&r2=227481&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/Target/<u></u>X86/X86FastISel.cpp?rev=<u></u>227481&r1=227480&r2=227481&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/Target/X86/<u></u>X86FastISel.cpp (original)<br>
+++ llvm/trunk/lib/Target/X86/<u></u>X86FastISel.cpp Thu Jan 29 13:09:18 2015<br>
@@ -80,7 +80,7 @@ public:<br>
#include "X86GenFastISel.inc"<br>
<br>
private:<br>
- bool X86FastEmitCompare(const Value *LHS, const Value *RHS, EVT VT);<br>
+ bool X86FastEmitCompare(const Value *LHS, const Value *RHS, EVT VT, DebugLoc DL);<br>
<br>
bool X86FastEmitLoad(EVT VT, const X86AddressMode &AM, MachineMemOperand *MMO,<br>
unsigned &ResultReg);<br>
@@ -1109,7 +1109,7 @@ static unsigned X86ChooseCmpImmediateOpc<br>
}<br>
<br>
bool X86FastISel::<u></u>X86FastEmitCompare(const Value *Op0, const Value *Op1,<br>
- EVT VT) {<br>
+ EVT VT, DebugLoc CurDbgLoc) {<br>
unsigned Op0Reg = getRegForValue(Op0);<br>
if (Op0Reg == 0) return false;<br>
<br>
@@ -1122,7 +1122,7 @@ bool X86FastISel::<u></u>X86FastEmitCompare(con<br>
// CMPri, otherwise use CMPrr.<br>
if (const ConstantInt *Op1C = dyn_cast<ConstantInt>(Op1)) {<br>
if (unsigned CompareImmOpc = X86ChooseCmpImmediateOpcode(<u></u>VT, Op1C)) {<br>
- BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(CompareImmOpc))<br>
+ BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, CurDbgLoc, TII.get(CompareImmOpc))<br>
.addReg(Op0Reg)<br>
.addImm(Op1C->getSExtValue());<br>
return true;<br>
@@ -1134,7 +1134,7 @@ bool X86FastISel::<u></u>X86FastEmitCompare(con<br>
<br>
unsigned Op1Reg = getRegForValue(Op1);<br>
if (Op1Reg == 0) return false;<br>
- BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(CompareOpc))<br>
+ BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, CurDbgLoc, TII.get(CompareOpc))<br>
.addReg(Op0Reg)<br>
.addReg(Op1Reg);<br>
<br>
@@ -1202,7 +1202,7 @@ bool X86FastISel::X86SelectCmp(<u></u>const Ins<br>
<br>
ResultReg = createResultReg(&X86::<u></u>GR8RegClass);<br>
if (SETFOpc) {<br>
- if (!X86FastEmitCompare(LHS, RHS, VT))<br>
+ if (!X86FastEmitCompare(LHS, RHS, VT, I->getDebugLoc()))<br>
return false;<br>
<br>
unsigned FlagReg1 = createResultReg(&X86::<u></u>GR8RegClass);<br>
@@ -1227,7 +1227,7 @@ bool X86FastISel::X86SelectCmp(<u></u>const Ins<br>
std::swap(LHS, RHS);<br>
<br>
// Emit a compare of LHS/RHS.<br>
- if (!X86FastEmitCompare(LHS, RHS, VT))<br>
+ if (!X86FastEmitCompare(LHS, RHS, VT, I->getDebugLoc()))<br>
return false;<br>
<br>
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc), ResultReg);<br>
@@ -1353,7 +1353,7 @@ bool X86FastISel::X86SelectBranch(<u></u>const<br>
std::swap(CmpLHS, CmpRHS);<br>
<br>
// Emit a compare of the LHS and RHS, setting the flags.<br>
- if (!X86FastEmitCompare(CmpLHS, CmpRHS, VT))<br>
+ if (!X86FastEmitCompare(CmpLHS, CmpRHS, VT, CI->getDebugLoc()))<br>
return false;<br>
<br>
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(BranchOpc))<br>
@@ -1740,7 +1740,7 @@ bool X86FastISel::<u></u>X86FastEmitCMoveSelect<br>
<br>
EVT CmpVT = TLI.getValueType(CmpLHS-><u></u>getType());<br>
// Emit a compare of the LHS and RHS, setting the flags.<br>
- if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT))<br>
+ if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT, CI->getDebugLoc()))<br>
return false;<br>
<br>
if (SETFOpc) {<br>
@@ -1924,7 +1924,7 @@ bool X86FastISel::<u></u>X86FastEmitPseudoSelec<br>
std::swap(CmpLHS, CmpRHS);<br>
<br>
EVT CmpVT = TLI.getValueType(CmpLHS-><u></u>getType());<br>
- if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT))<br>
+ if (!X86FastEmitCompare(CmpLHS, CmpRHS, CmpVT, CI->getDebugLoc()))<br>
return false;<br>
} else {<br>
unsigned CondReg = getRegForValue(Cond);<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</div></div></blockquote></div></div></div></blockquote></div></div>