<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 13, 2014, at 12:36 PM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, May 13, 2014 at 10:04:28AM -0700, Pete Cooper wrote:<br><blockquote type="cite"><br>On May 13, 2014, at 9:00 AM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:<br><br><blockquote type="cite">On Mon, May 12, 2014 at 03:53:22PM -0700, Pete Cooper wrote:<br><blockquote type="cite">Hi Tom<br><br>Sorry for the late comments.<br><br>I can’t speak for the targets themselves but the LegalizeDAG part looks good, with just one comment...<br>On May 5, 2014, at 8:17 PM, Tom Stellard <<a href="mailto:thomas.stellard@amd.com">thomas.stellard@amd.com</a>> wrote:<br><br><blockquote type="cite">This consolidates code from the Hexagon, R600, and XCore targets.<br><br>No functionality change intended.<br>---<br>lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | 15 +++++++++++++++<br>lib/Target/Hexagon/HexagonISelLowering.cpp | 20 ++------------------<br>lib/Target/Hexagon/HexagonISelLowering.h | 1 -<br>lib/Target/R600/SIISelLowering.cpp | 22 ++++------------------<br>lib/Target/R600/SIISelLowering.h | 1 -<br>lib/Target/XCore/XCoreISelLowering.cpp | 13 +------------<br>lib/Target/XCore/XCoreISelLowering.h | 1 -<br>7 files changed, 22 insertions(+), 51 deletions(-)<br><br>diff --git a/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp<br>index dac372f..745ed4a 100644<br>--- a/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp<br>+++ b/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp<br>@@ -3899,8 +3899,23 @@ void SelectionDAGLegalize::ExpandNode(SDNode *Node) {<br> Tmp2 = Node->getOperand(1); // RHS<br> Tmp3 = Node->getOperand(2); // True<br> Tmp4 = Node->getOperand(3); // False<br>+ EVT VT = Node->getValueType(0);<br> SDValue CC = Node->getOperand(4);<br><br>+ // First check if SELECT_CC nodes are legal for this type<br>+ if (!TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT)) {<br>+ EVT CmpVT = Tmp1.getValueType();<br>+ assert(TLI.isOperationLegalOrCustom(ISD::SELECT, VT) &&<br>+ TLI.isOperationLegalOrCustom(ISD::SETCC, CmpVT) &&<br></blockquote>It looks like ExpandNode could handle SETCC being illegal here. An illegal SETCC will be further expanded to a SELET_CC. So we may want to remove the SETCC assert if you’re happy that we won’t hit infinite legalization. I don’t think we will, but its hard to be 100% sure.<br><br></blockquote><br>Hi Pete,<br><br>I think it will lead to an infinite loop if we drop the assert, because<br>if the SETCC is lowered to an illegal SELECT_CC, then we will be right<br>back here where we started.<br></blockquote>It all depends on the type of CCVT. You may have something like<br><br>i32 selectcc = i8 cond, i32 true, i32 false<br><br>If you have i8 selectcc legal then you could have safely created the i8 setcc even though its not legal or custom.<br><br></blockquote><br>That's right, so if we remove the assert, there is a chance that someone<br>could hit an infinite loop. I think that's better than incorrectly<br>asserting when we could legalize something, and it will be very<br>difficult to detect when the infinite loop may occur.<br></div></blockquote>Yeah, its a pain we can’t do a better job of detecting the infinite loop. Thats not something to block this patch though, just an observation on legalization itself.</div><div><br></div><div>The target independent bit LGTM.</div><div><br></div><div>Thanks,</div><div>Pete<br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>Here is an updated patch with the assert removed.<br><br>-Tom<br><br><br><blockquote type="cite">Thanks,<br>Pete<br><blockquote type="cite"><br>I did notice a different issue with the patch. We can't check for<br><br>if (!TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT)) {<br><br>Because we may need to Expand an node that is marked Custom in case the<br>target does nothing to the node in its custom lowering function.<br>Instead we must use:<br><br>if (TLI.isCondCodeLegal(CCOp, Tmp1.getSimpleValueType()))<br><br><br>Also, cc'ing code owners in case they have any comments.<br><br>-Tom<br><br><blockquote type="cite">Thanks,<br>Pete<br><blockquote type="cite">+ "ISD::SELECT and ISD::SETCC must both be legal in order to expand "<br>+ "ISD::SELECT_CC");<br>+ EVT CCVT = TLI.getSetCCResultType(*DAG.getContext(), CmpVT);<br>+ SDValue Cond = DAG.getNode(ISD::SETCC, dl, CCVT, Tmp1, Tmp2, CC);<br>+ Results.push_back(DAG.getSelect(dl, VT, Cond, Tmp3, Tmp4));<br>+ break;<br>+ }<br>+<br>+ // SELECT_CC is legal, so the condition code must not be.<br> bool Legalized = false;<br> // Try to legalize by inverting the condition. This is for targets that<br> // might support an ordered version of a condition, but not the unordered<br>diff --git a/lib/Target/Hexagon/HexagonISelLowering.cpp b/lib/Target/Hexagon/HexagonISelLowering.cpp<br>index b8e5d24..aa4e9b5 100644<br>--- a/lib/Target/Hexagon/HexagonISelLowering.cpp<br>+++ b/lib/Target/Hexagon/HexagonISelLowering.cpp<br>@@ -944,21 +944,6 @@ HexagonTargetLowering::LowerVASTART(SDValue Op, SelectionDAG &DAG) const {<br>}<br><br>SDValue<br>-HexagonTargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {<br>- SDValue LHS = Op.getOperand(0);<br>- SDValue RHS = Op.getOperand(1);<br>- SDValue CC = Op.getOperand(4);<br>- SDValue TrueVal = Op.getOperand(2);<br>- SDValue FalseVal = Op.getOperand(3);<br>- SDLoc dl(Op);<br>- SDNode* OpNode = Op.getNode();<br>- EVT SVT = OpNode->getValueType(0);<br>-<br>- SDValue Cond = DAG.getNode(ISD::SETCC, dl, MVT::i1, LHS, RHS, CC);<br>- return DAG.getNode(ISD::SELECT, dl, SVT, Cond, TrueVal, FalseVal);<br>-}<br>-<br>-SDValue<br>HexagonTargetLowering::LowerConstantPool(SDValue Op, SelectionDAG &DAG) const {<br>EVT ValTy = Op.getValueType();<br>SDLoc dl(Op);<br>@@ -1341,8 +1326,8 @@ HexagonTargetLowering::HexagonTargetLowering(HexagonTargetMachine<br> setOperationAction(ISD::BSWAP, MVT::i64, Expand);<br><br> // Lower SELECT_CC to SETCC and SELECT.<br>- setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);<br>- setOperationAction(ISD::SELECT_CC, MVT::i64, Custom);<br>+ setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);<br>+ setOperationAction(ISD::SELECT_CC, MVT::i64, Expand);<br><br> if (QRI->Subtarget.hasV5TOps()) {<br><br>@@ -1577,7 +1562,6 @@ HexagonTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {<br> case ISD::BR_JT: return LowerBR_JT(Op, DAG);<br><br> case ISD::DYNAMIC_STACKALLOC: return LowerDYNAMIC_STACKALLOC(Op, DAG);<br>- case ISD::SELECT_CC: return LowerSELECT_CC(Op, DAG);<br> case ISD::SELECT: return Op;<br> case ISD::INTRINSIC_WO_CHAIN: return LowerINTRINSIC_WO_CHAIN(Op, DAG);<br> case ISD::INLINEASM: return LowerINLINEASM(Op, DAG);<br>diff --git a/lib/Target/Hexagon/HexagonISelLowering.h b/lib/Target/Hexagon/HexagonISelLowering.h<br>index 4f27c27..0ddaf84 100644<br>--- a/lib/Target/Hexagon/HexagonISelLowering.h<br>+++ b/lib/Target/Hexagon/HexagonISelLowering.h<br>@@ -124,7 +124,6 @@ namespace llvm {<br> const SmallVectorImpl<SDValue> &OutVals,<br> SDValue Callee) const;<br><br>- SDValue LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerFRAMEADDR(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerATOMIC_FENCE(SDValue Op, SelectionDAG& DAG) const;<br> SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;<br>diff --git a/lib/Target/R600/SIISelLowering.cpp b/lib/Target/R600/SIISelLowering.cpp<br>index cacff83..57ac274 100644<br>--- a/lib/Target/R600/SIISelLowering.cpp<br>+++ b/lib/Target/R600/SIISelLowering.cpp<br>@@ -103,10 +103,10 @@ SITargetLowering::SITargetLowering(TargetMachine &TM) :<br>setOperationAction(ISD::SELECT, MVT::f64, Promote);<br>AddPromotedToType(ISD::SELECT, MVT::f64, MVT::i64);<br><br>- setOperationAction(ISD::SELECT_CC, MVT::f32, Custom);<br>- setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);<br>-<br>- setOperationAction(ISD::SELECT_CC, MVT::Other, Expand);<br>+ setOperationAction(ISD::SELECT_CC, MVT::f32, Expand);<br>+ setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);<br>+ setOperationAction(ISD::SELECT_CC, MVT::i64, Expand);<br>+ setOperationAction(ISD::SELECT_CC, MVT::f64, Expand);<br><br>setOperationAction(ISD::SETCC, MVT::v2i1, Expand);<br>setOperationAction(ISD::SETCC, MVT::v4i1, Expand);<br>@@ -601,7 +601,6 @@ SDValue SITargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {<br>}<br><br>case ISD::SELECT: return LowerSELECT(Op, DAG);<br>- case ISD::SELECT_CC: return LowerSELECT_CC(Op, DAG);<br>case ISD::SIGN_EXTEND: return LowerSIGN_EXTEND(Op, DAG);<br>case ISD::STORE: return LowerSTORE(Op, DAG);<br>case ISD::ANY_EXTEND: // Fall-through<br>@@ -893,19 +892,6 @@ SDValue SITargetLowering::LowerSELECT(SDValue Op, SelectionDAG &DAG) const {<br>return DAG.getNode(ISD::BITCAST, DL, MVT::i64, Res);<br>}<br><br>-SDValue SITargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {<br>- SDValue LHS = Op.getOperand(0);<br>- SDValue RHS = Op.getOperand(1);<br>- SDValue True = Op.getOperand(2);<br>- SDValue False = Op.getOperand(3);<br>- SDValue CC = Op.getOperand(4);<br>- EVT VT = Op.getValueType();<br>- SDLoc DL(Op);<br>-<br>- SDValue Cond = DAG.getNode(ISD::SETCC, DL, MVT::i1, LHS, RHS, CC);<br>- return DAG.getNode(ISD::SELECT, DL, VT, Cond, True, False);<br>-}<br>-<br>SDValue SITargetLowering::LowerSIGN_EXTEND(SDValue Op,<br> SelectionDAG &DAG) const {<br>EVT VT = Op.getValueType();<br>diff --git a/lib/Target/R600/SIISelLowering.h b/lib/Target/R600/SIISelLowering.h<br>index c6eaa81..3253f21 100644<br>--- a/lib/Target/R600/SIISelLowering.h<br>+++ b/lib/Target/R600/SIISelLowering.h<br>@@ -27,7 +27,6 @@ class SITargetLowering : public AMDGPUTargetLowering {<br> SelectionDAG &DAG) const;<br>SDValue LowerLOAD(SDValue Op, SelectionDAG &DAG) const;<br>SDValue LowerSELECT(SDValue Op, SelectionDAG &DAG) const;<br>- SDValue LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;<br>SDValue LowerSIGN_EXTEND(SDValue Op, SelectionDAG &DAG) const;<br>SDValue LowerSTORE(SDValue Op, SelectionDAG &DAG) const;<br>SDValue LowerZERO_EXTEND(SDValue Op, SelectionDAG &DAG) const;<br>diff --git a/lib/Target/XCore/XCoreISelLowering.cpp b/lib/Target/XCore/XCoreISelLowering.cpp<br>index 1c52c70..96eebb4 100644<br>--- a/lib/Target/XCore/XCoreISelLowering.cpp<br>+++ b/lib/Target/XCore/XCoreISelLowering.cpp<br>@@ -92,7 +92,7 @@ XCoreTargetLowering::XCoreTargetLowering(XCoreTargetMachine &XTM)<br><br>// XCore does not have the NodeTypes below.<br>setOperationAction(ISD::BR_CC, MVT::i32, Expand);<br>- setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);<br>+ setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);<br>setOperationAction(ISD::ADDC, MVT::i32, Expand);<br>setOperationAction(ISD::ADDE, MVT::i32, Expand);<br>setOperationAction(ISD::SUBC, MVT::i32, Expand);<br>@@ -217,7 +217,6 @@ LowerOperation(SDValue Op, SelectionDAG &DAG) const {<br>case ISD::BR_JT: return LowerBR_JT(Op, DAG);<br>case ISD::LOAD: return LowerLOAD(Op, DAG);<br>case ISD::STORE: return LowerSTORE(Op, DAG);<br>- case ISD::SELECT_CC: return LowerSELECT_CC(Op, DAG);<br>case ISD::VAARG: return LowerVAARG(Op, DAG);<br>case ISD::VASTART: return LowerVASTART(Op, DAG);<br>case ISD::SMUL_LOHI: return LowerSMUL_LOHI(Op, DAG);<br>@@ -259,16 +258,6 @@ void XCoreTargetLowering::ReplaceNodeResults(SDNode *N,<br>//===----------------------------------------------------------------------===//<br><br>SDValue XCoreTargetLowering::<br>-LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const<br>-{<br>- SDLoc dl(Op);<br>- SDValue Cond = DAG.getNode(ISD::SETCC, dl, MVT::i32, Op.getOperand(2),<br>- Op.getOperand(3), Op.getOperand(4));<br>- return DAG.getNode(ISD::SELECT, dl, MVT::i32, Cond, Op.getOperand(0),<br>- Op.getOperand(1));<br>-}<br>-<br>-SDValue XCoreTargetLowering::<br>getGlobalAddressWrapper(SDValue GA, const GlobalValue *GV,<br> SelectionDAG &DAG) const<br>{<br>diff --git a/lib/Target/XCore/XCoreISelLowering.h b/lib/Target/XCore/XCoreISelLowering.h<br>index 4e662fc..fc8b364 100644<br>--- a/lib/Target/XCore/XCoreISelLowering.h<br>+++ b/lib/Target/XCore/XCoreISelLowering.h<br>@@ -157,7 +157,6 @@ namespace llvm {<br> SDValue LowerBlockAddress(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerBR_JT(SDValue Op, SelectionDAG &DAG) const;<br>- SDValue LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerVAARG(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerVASTART(SDValue Op, SelectionDAG &DAG) const;<br> SDValue LowerUMUL_LOHI(SDValue Op, SelectionDAG &DAG) const;<br>--<span class="Apple-converted-space"> </span><br>1.8.1.5<br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch><br></blockquote><br></blockquote><span><0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch></span></div></blockquote></div><br></body></html>