<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;">Hey Tom<div><br></div><div>Thanks, using Expand here makes sense.</div><div><br></div><div>So everything LGTM.</div><div><br></div><div>Thanks,</div><div>Pete<br><div><div>On Jun 9, 2014, at 1:45 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 12:39:43PM -0700, Pete Cooper wrote:<br><blockquote type="cite"><br>On May 13, 2014, at 12:36 PM, Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>> wrote:<br><br><blockquote type="cite">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></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.<br><br>The target independent bit LGTM.<br><br></blockquote><br>Hi Pete,<br><br>I've done more testing with this, and I think the assert should be testing:<br><br>!TLI.isOperationExpand(ISD::SELECT, VT)<br><br>We should be able to Expand SELECT_CC if SELECT is marked<br>as Promote.  For example, on R600, we promote f32 SELECT to i32 SELECT.<br>I have attached an update patch.<br><br>Thanks,<br>Tom<br><br><blockquote type="cite">Thanks,<br>Pete<br><blockquote type="cite"><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><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></div></body></html>