[LegalizeDAG] Truncate condition operand of ISD::SELECT

[evgeny777]

Summary

I'm facing a crash when compiling large in-house project for AArch64 using clang, which happens because condition argument of ISD::SELECT can't be expanded (for my case required expansion is i128 -> i64). This patch adds required support.

[efriedma]

It seems a little dubious to create a SELECT where the condition isn't either an i1 or a naturally promoted i1... maybe it would make sense to restrict that instead? (And fix the VSELECT expansion to just truncate the condition instead of trying to get fancy.)

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

3118

Probably want to clarify that you're intentionally throwing away the high bits because the operand is required to conform to getBooleanContents.

[evgeny777]

Addressed review comments. The new version also handles scalar types which size is not power of 2.

[evgeny777]

Any comments on this? @efriedma ?

[delena]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

368 ↗

(On Diff #132335)

Can the condition be wider than getSetCCResultType?

test/CodeGen/AArch64/expand-select.ll

5

I think you should put here regular CHECKs, like in all other llc tests.

[evgeny777]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

368 ↗

(On Diff #132335)

Yes, if this wider type is legal for target architecture. However see @efriedma comment above.

If boolean type is not legal then you either crash when trying to expand condition or when trying to promote it (if type bit width is not power of 2). Here I chose to truncate it to size of getSetCCResultType to avoid promoting it back in PromoteTargetBoolean.

test/CodeGen/AArch64/expand-select.ll

5

Any suggestion on what should be checked? Probably debug output from -debug-only=selectiondag,isel?

[delena]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

368 ↗

(On Diff #132335)

My question was incorrect. I wanted to ask whether the getSetCCResultType may be wider than Cond. Do you need to extend the Cond in this case?

test/CodeGen/AArch64/expand-select.ll

5

just use utils/update_llc_test_checks.py to generate "CHECKS":

utils/update_llc_test_checks.py --llc-binary=<path>/llc test/CodeGen/AArch64/expand-select.ll

[evgeny777]

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

368 ↗

(On Diff #132335)

I don't think so. PromoteIntOp_SELECT will do this for me. It calls PromoteTargetBoolean which in turn zero extends condition to the size of getSetCCResultType

test/CodeGen/AArch64/expand-select.ll

5

Thanks, I'll give it a try.

[evgeny777]

Added checks to test case

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

#endif
     llvm_unreachable("Do not know how to expand this operator's operand!");
 
   case ISD::BITCAST:           Res = ExpandOp_BITCAST(N); break;
   case ISD::BR_CC:             Res = ExpandIntOp_BR_CC(N); break;
   case ISD::BUILD_VECTOR:      Res = ExpandOp_BUILD_VECTOR(N); break;
   case ISD::EXTRACT_ELEMENT:   Res = ExpandOp_EXTRACT_ELEMENT(N); break;
   case ISD::INSERT_VECTOR_ELT: Res = ExpandOp_INSERT_VECTOR_ELT(N); break;
   case ISD::SCALAR_TO_VECTOR:  Res = ExpandOp_SCALAR_TO_VECTOR(N); break;
+  case ISD::SELECT:
+    // We expect only condition argument to be expanded
+    assert(OpNo == 0);
+    Res = ExpandIntOp_SELECT(N);
+    break;
   case ISD::SELECT_CC:         Res = ExpandIntOp_SELECT_CC(N); break;
   case ISD::SETCC:             Res = ExpandIntOp_SETCC(N); break;
   case ISD::SETCCE:            Res = ExpandIntOp_SETCCE(N); break;
   case ISD::SETCCCARRY:        Res = ExpandIntOp_SETCCCARRY(N); break;
   case ISD::SINT_TO_FP:        Res = ExpandIntOp_SINT_TO_FP(N); break;
   case ISD::STORE:   Res = ExpandIntOp_STORE(cast<StoreSDNode>(N), OpNo); break;
   case ISD::TRUNCATE:          Res = ExpandIntOp_TRUNCATE(N); break;
   case ISD::UINT_TO_FP:        Res = ExpandIntOp_UINT_TO_FP(N); break;
SDValue DAGTypeLegalizer::ExpandIntOp_BR_CC(SDNode *N) {
   }
 
   // Update N to have the operands specified.
   return SDValue(DAG.UpdateNodeOperands(N, N->getOperand(0),
                                 DAG.getCondCode(CCCode), NewLHS, NewRHS,
                                 N->getOperand(4)), 0);
 }
 
+SDValue DAGTypeLegalizer::ExpandIntOp_SELECT(SDNode *N) {
+  SDValue InL, InH;
+  GetExpandedInteger(N->getOperand(0), InL, InH);
+  return DAG.getNode(ISD::SELECT, SDLoc(N), N->getValueType(0), InL,
+                     N->getOperand(1), N->getOperand(2));
+}
+
 SDValue DAGTypeLegalizer::ExpandIntOp_SELECT_CC(SDNode *N) {
   SDValue NewLHS = N->getOperand(0), NewRHS = N->getOperand(1);
   ISD::CondCode CCCode = cast<CondCodeSDNode>(N->getOperand(4))->get();
   IntegerExpandSetCCOperands(NewLHS, NewRHS, CCCode, SDLoc(N));
 
   // If ExpandSetCCOperands returned a scalar, we need to compare the result
   // against zero to select between true and false values.
   if (!NewRHS.getNode()) {

lib/CodeGen/SelectionDAG/LegalizeTypes.h

private:
   void ExpandShiftByConstant(SDNode *N, const APInt &Amt,
                              SDValue &Lo, SDValue &Hi);
   bool ExpandShiftWithKnownAmountBit(SDNode *N, SDValue &Lo, SDValue &Hi);
   bool ExpandShiftWithUnknownAmountBit(SDNode *N, SDValue &Lo, SDValue &Hi);
 
   // Integer Operand Expansion.
   bool ExpandIntegerOperand(SDNode *N, unsigned OperandNo);
   SDValue ExpandIntOp_BR_CC(SDNode *N);
+  SDValue ExpandIntOp_SELECT(SDNode *N);
   SDValue ExpandIntOp_SELECT_CC(SDNode *N);
   SDValue ExpandIntOp_SETCC(SDNode *N);
   SDValue ExpandIntOp_SETCCE(SDNode *N);
   SDValue ExpandIntOp_SETCCCARRY(SDNode *N);
   SDValue ExpandIntOp_Shift(SDNode *N);
   SDValue ExpandIntOp_SINT_TO_FP(SDNode *N);
   SDValue ExpandIntOp_STORE(StoreSDNode *N, unsigned OpNo);
   SDValue ExpandIntOp_TRUNCATE(SDNode *N);

test/CodeGen/AArch64/expand-select.ll

+; REQUIRES: asserts
+
+; Check that we don't crash
+; RUN: llc -mtriple=aarch64-unknown-linux-gnu -O3 %s -debug-only=selectiondag,legalize-types -o -
+
+define void @foo(i32 %In1, <2 x i128> %In2, <2 x i128> %In3, <2 x i128> *%Out) {
+  %cond = and i32 %In1, 1
+  %cbool = icmp eq i32 %cond, 0
+  %res = select i1 %cbool, <2 x i128> %In2, <2 x i128> %In3
+  store <2 x i128> %res, <2 x i128> *%Out
+ 
+  ret void
+}