[PowerPC] Fix the spill issue exposed by r310809

[nemanjai]

Summary

When I committed r310809, I didn't realize that it exposed a bug in the compare lowering I committed previously. Namely the issue is that compares of i32 values used code that produces 64-bit immediate results. When the register allocator spilled one of those intermediate values, it would spill/reload it as a 32-bit value. The sign bits are then lost and the comparison produces incorrect results.

This patch is basically r310809 but with the addition of the fix for the spilling issue. Namely, we now ensure that if the intermediate results are 64-bit values, the value is changed to an i64 so that it is spilled/reloaded as a 64-bit entity.

[nemanjai]

Updated the patch to only include the bug fix that r310809 needs in order to function correctly. This won't apply cleanly to trunk because r310809 was pulled out, so this needs to apply on top of that.

[nemanjai]

Add a test case.

[hfinkel]

I think this generally looks fine.

Just as a general note on code organizations and commenting, we seem to be accumulating a pretty deep call tree here of functions that only work correctly in 64-bit mode. Not in this patch, but we should do something about this (either segregating these things into a distinct part of the file and/or adding asserts and/or comments to the 64-bit-mode-only functions.

[echristo]

In D38054#877711, @hfinkel wrote:

I think this generally looks fine.

Just as a general note on code organizations and commenting, we seem to be accumulating a pretty deep call tree here of functions that only work correctly in 64-bit mode. Not in this patch, but we should do something about this (either segregating these things into a distinct part of the file and/or adding asserts and/or comments to the 64-bit-mode-only functions.

Agreed. Also a lot of the fine points of when to extend and when not could probably be split out as well. This is definitely getting pretty unwieldy.

-eric

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

2979–2983

As a note I find the ZeroCompare enum to be fairly inscrutable. Perhaps document it and what each value is?

[nemanjai]

In D38054#878445, @echristo wrote:

In D38054#877711, @hfinkel wrote:

I think this generally looks fine.

Just as a general note on code organizations and commenting, we seem to be accumulating a pretty deep call tree here of functions that only work correctly in 64-bit mode. Not in this patch, but we should do something about this (either segregating these things into a distinct part of the file and/or adding asserts and/or comments to the 64-bit-mode-only functions.

Agreed. Also a lot of the fine points of when to extend and when not could probably be split out as well. This is definitely getting pretty unwieldy.

-eric

OK. I'll commit this patch as-is with the inline comment addressed.

Then perhaps in a subsequent patch, I'll create something like class GPRComparisonSelector that uses essentially the same approach as the BitPermutationSelector. The Select() function will do the necessary checks and exit early if we're not a PPC64 target. All the functions that do the actual selection will be private. This way we should be able to avoid carelessly calling these functions under invalid conditions.
Does that sound like a good plan?

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

2979–2983

Will do.

[hfinkel]

In D38054#878633, @nemanjai wrote:

In D38054#878445, @echristo wrote:

In D38054#877711, @hfinkel wrote:

I think this generally looks fine.

Just as a general note on code organizations and commenting, we seem to be accumulating a pretty deep call tree here of functions that only work correctly in 64-bit mode. Not in this patch, but we should do something about this (either segregating these things into a distinct part of the file and/or adding asserts and/or comments to the 64-bit-mode-only functions.

Agreed. Also a lot of the fine points of when to extend and when not could probably be split out as well. This is definitely getting pretty unwieldy.

-eric

OK. I'll commit this patch as-is with the inline comment addressed.

Then perhaps in a subsequent patch, I'll create something like class GPRComparisonSelector that uses essentially the same approach as the BitPermutationSelector. The Select() function will do the necessary checks and exit early if we're not a PPC64 target. All the functions that do the actual selection will be private. This way we should be able to avoid carelessly calling these functions under invalid conditions.
Does that sound like a good plan?

Yes, I think that would be better.

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

 #define DEBUG_TYPE "ppc-codegen"
 
 STATISTIC(NumSextSetcc,
           "Number of (sext(setcc)) nodes expanded into GPR sequence.");
 STATISTIC(NumZextSetcc,
           "Number of (zext(setcc)) nodes expanded into GPR sequence.");
 STATISTIC(SignExtensionsAdded,
           "Number of sign extensions for compare inputs added.");
-STATISTIC(ZeroExtensionsAdded,
-          "Number of zero extensions for compare inputs added.");
 STATISTIC(NumLogicOpsOnComparison,
           "Number of logical ops on i1 values calculated in GPR.");
 STATISTIC(OmittedForNonExtendUses,
           "Number of compares not eliminated as they have non-extending uses.");
 
 // FIXME: Remove this once the bug has been fixed!
 cl::opt<bool> ANDIGlueBug("expose-ppc-andi-glue-bug",
 cl::desc("expose the ANDI glue bug on PPC"), cl::Hidden);
private:
     // For example (a >= 0) == (a > -1).
     enum ZeroCompare { GEZExt, GESExt, LEZExt, LESExt };
 
     bool trySETCC(SDNode *N);
     bool tryEXTEND(SDNode *N);
     bool tryLogicOpOfCompares(SDNode *N);
     SDValue computeLogicOpInGPR(SDValue LogicOp);
     SDValue signExtendInputIfNeeded(SDValue Input);
-    SDValue zeroExtendInputIfNeeded(SDValue Input);
     SDValue addExtOrTrunc(SDValue NatWidthRes, ExtOrTruncConversion Conv);
     SDValue getCompoundZeroComparisonInGPR(SDValue LHS, SDLoc dl,
                                            ZeroCompare CmpTy);
     SDValue get32BitZExtCompare(SDValue LHS, SDValue RHS, ISD::CondCode CC,
                                 int64_t RHSValue, SDLoc dl);
     SDValue get32BitSExtCompare(SDValue LHS, SDValue RHS, ISD::CondCode CC,
                                 int64_t RHSValue, SDLoc dl);
     SDValue get64BitZExtCompare(SDValue LHS, SDValue RHS, ISD::CondCode CC,
WideRes =
       getSETCCInGPR(N->getOperand(0),
                     N->getOpcode() == ISD::SIGN_EXTEND ?
                     SetccInGPROpts::SExtOrig : SetccInGPROpts::ZExtOrig);
 
   if (!WideRes)
     return false;
 
   SDLoc dl(N);
-  bool Inputs32Bit = N->getOperand(0).getOperand(0).getValueType() == MVT::i32;
+  bool Input32Bit = WideRes.getValueType() == MVT::i32;
   bool Output32Bit = N->getValueType(0) == MVT::i32;
 
   NumSextSetcc += N->getOpcode() == ISD::SIGN_EXTEND ? 1 : 0;
   NumZextSetcc += N->getOpcode() == ISD::SIGN_EXTEND ? 0 : 1;
 
   SDValue ConvOp = WideRes;
-  if (Inputs32Bit != Output32Bit)
-    ConvOp = addExtOrTrunc(WideRes, Inputs32Bit ? ExtOrTruncConversion::Ext :
+  if (Input32Bit != Output32Bit)
+    ConvOp = addExtOrTrunc(WideRes, Input32Bit ? ExtOrTruncConversion::Ext :
                            ExtOrTruncConversion::Trunc);
   ReplaceNode(N, ConvOp.getNode());
 
   return true;
 }
 
 // Lower a logical operation on i1 values into a GPR sequence if possible.
 // The result can be kept in a GPR if requested.
SDValue CRBit =
     SDValue(CurDAG->getMachineNode(TargetOpcode::EXTRACT_SUBREG, dl,
                                    MVT::i1, CR0Reg, SRIdxVal,
                                    WideOp.getValue(1)), 0);
   ReplaceNode(N, CRBit.getNode());
   return true;
 }
 
 /// If the value isn't guaranteed to be sign-extended to 64-bits, extend it.
+/// Otherwise just reinterpret it as a 64-bit value.
 /// Useful when emitting comparison code for 32-bit values without using
 /// the compare instruction (which only considers the lower 32-bits).
 SDValue PPCDAGToDAGISel::signExtendInputIfNeeded(SDValue Input) {
   assert(Input.getValueType() == MVT::i32 &&
          "Can only sign-extend 32-bit values here.");
   unsigned Opc = Input.getOpcode();
 
   // The value was sign extended and then truncated to 32-bits. No need to
   // sign extend it again.
   if (Opc == ISD::TRUNCATE &&
       (Input.getOperand(0).getOpcode() == ISD::AssertSext ||
        Input.getOperand(0).getOpcode() == ISD::SIGN_EXTEND))
-    return Input;
+    return addExtOrTrunc(Input, ExtOrTruncConversion::Ext);
 
   LoadSDNode *InputLoad = dyn_cast<LoadSDNode>(Input);
   // The input is a sign-extending load. No reason to sign-extend.
   if (InputLoad && InputLoad->getExtensionType() == ISD::SEXTLOAD)
-    return Input;
+    return addExtOrTrunc(Input, ExtOrTruncConversion::Ext);
 
   ConstantSDNode *InputConst = dyn_cast<ConstantSDNode>(Input);
   // We don't sign-extend constants and already sign-extended values.
   if (InputConst || Opc == ISD::AssertSext || Opc == ISD::SIGN_EXTEND_INREG ||
       Opc == ISD::SIGN_EXTEND)
-    return Input;
+    return addExtOrTrunc(Input, ExtOrTruncConversion::Ext);
 
   SDLoc dl(Input);
   SignExtensionsAdded++;
-  return SDValue(CurDAG->getMachineNode(PPC::EXTSW_32, dl, MVT::i32, Input), 0);
-}
-
-/// If the value isn't guaranteed to be zero-extended to 64-bits, extend it.
-/// Useful when emitting comparison code for 32-bit values without using
-/// the compare instruction (which only considers the lower 32-bits).
-SDValue PPCDAGToDAGISel::zeroExtendInputIfNeeded(SDValue Input) {
-  assert(Input.getValueType() == MVT::i32 &&
-         "Can only zero-extend 32-bit values here.");
-  LoadSDNode *InputLoad = dyn_cast<LoadSDNode>(Input);
-  unsigned Opc = Input.getOpcode();
-
-  // No need to zero-extend loaded values (unless they're loaded with
-  // a sign-extending load).
-  if (InputLoad && InputLoad->getExtensionType() != ISD::SEXTLOAD)
-    return Input;
-
-  ConstantSDNode *InputConst = dyn_cast<ConstantSDNode>(Input);
-  bool InputZExtConst = InputConst && InputConst->getSExtValue() >= 0;
-  // An ISD::TRUNCATE will be lowered to an EXTRACT_SUBREG so we have
-  // to conservatively actually clear the high bits. We also don't need to
-  // zero-extend constants or values that are already zero-extended.
-  if (InputZExtConst || Opc == ISD::AssertZext || Opc == ISD::ZERO_EXTEND)
-    return Input;
-
-  SDLoc dl(Input);
-  ZeroExtensionsAdded++;
-  return SDValue(CurDAG->getMachineNode(PPC::RLDICL_32, dl, MVT::i32, Input,
-                                        getI64Imm(0, dl), getI64Imm(32, dl)),
-                 0);
+  return SDValue(CurDAG->getMachineNode(PPC::EXTSW_32_64, dl,
+                                        MVT::i64, Input), 0);
 }
 
 // Handle a 32-bit value in a 64-bit register and vice-versa. These are of
 // course not actual zero/sign extensions that will generate machine code,
 // they're just a way to reinterpret a 32 bit value in a register as a
 // 64 bit value and vice-versa.
 SDValue PPCDAGToDAGISel::addExtOrTrunc(SDValue NatWidthRes,
                                        ExtOrTruncConversion Conv) {
SDValue PPCDAGToDAGISel::getCompoundZeroComparisonInGPR(SDValue LHS, SDLoc dl,
   default: llvm_unreachable("Unknown Zero-comparison type.");
   case ZeroCompare::GEZExt:
   case ZeroCompare::GESExt:
     ToExtend = SDValue(CurDAG->getMachineNode(Is32Bit ? PPC::NOR : PPC::NOR8,
                                               dl, InVT, LHS, LHS), 0);
   case ZeroCompare::LEZExt:
   case ZeroCompare::LESExt: {
     if (Is32Bit) {
+      // Upper 32 bits cannot be undefined for this sequence.
+      LHS = signExtendInputIfNeeded(LHS);
       SDValue Neg =
-        SDValue(CurDAG->getMachineNode(PPC::NEG, dl, MVT::i32, LHS), 0);
+        SDValue(CurDAG->getMachineNode(PPC::NEG8, dl, MVT::i64, LHS), 0);
       ToExtend =
-        SDValue(CurDAG->getMachineNode(PPC::RLDICL_32, dl, MVT::i32,
+        SDValue(CurDAG->getMachineNode(PPC::RLDICL, dl, MVT::i64,
                                        Neg, getI64Imm(1, dl),
                                        getI64Imm(63, dl)), 0);
     } else {
       SDValue Addi =
         SDValue(CurDAG->getMachineNode(PPC::ADDI8, dl, MVT::i64, LHS,
                                        getI64Imm(~0ULL, dl)), 0);
       ToExtend = SDValue(CurDAG->getMachineNode(PPC::OR8, dl, MVT::i64,
                                                 Addi, LHS), 0);
SDValue ShiftOps[] =
       { ToExtend, getI32Imm(1, dl), getI32Imm(31, dl), getI32Imm(31, dl) };
     return SDValue(CurDAG->getMachineNode(PPC::RLWINM, dl, MVT::i32,
                                           ShiftOps), 0);
   }
   case ZeroCompare::GESExt:
     return SDValue(CurDAG->getMachineNode(PPC::SRAWI, dl, MVT::i32, ToExtend,
                                           getI32Imm(31, dl)), 0);
   case ZeroCompare::LEZExt:
-    return SDValue(CurDAG->getMachineNode(PPC::XORI, dl, MVT::i32, ToExtend,
+    return SDValue(CurDAG->getMachineNode(PPC::XORI8, dl, MVT::i64, ToExtend,
                                           getI32Imm(1, dl)), 0);
   case ZeroCompare::LESExt:
-    return SDValue(CurDAG->getMachineNode(PPC::ADDI, dl, MVT::i32, ToExtend,
+    return SDValue(CurDAG->getMachineNode(PPC::ADDI8, dl, MVT::i64, ToExtend,
                                           getI32Imm(-1, dl)), 0);
   }
 }
 
 /// Produces a zero-extended result of comparing two 32-bit values according to
 /// the passed condition code.
 SDValue PPCDAGToDAGISel::get32BitZExtCompare(SDValue LHS, SDValue RHS,
                                              ISD::CondCode CC,
                                              int64_t RHSValue, SDLoc dl) {
   bool IsRHSZero = RHSValue == 0;
-  bool IsRHSOne = RHSValue == 1;
-  bool IsRHSNegOne = RHSValue == -1LL;
   switch (CC) {
   default: return SDValue();
   case ISD::SETEQ: {
     // (zext (setcc %a, %b, seteq)) -> (lshr (cntlzw (xor %a, %b)), 5)
     // (zext (setcc %a, 0, seteq))  -> (lshr (cntlzw %a), 5)
     SDValue Xor = IsRHSZero ? LHS :
       SDValue(CurDAG->getMachineNode(PPC::XOR, dl, MVT::i32, LHS, RHS), 0);
     SDValue Clz =
case ISD::SETGE: {
     IsRHSZero = RHSConst && RHSConst->isNullValue();
     LLVM_FALLTHROUGH;
   }
   case ISD::SETLE: {
     // (zext (setcc %a, %b, setle)) -> (xor (lshr (sub %b, %a), 63), 1)
     // (zext (setcc %a, 0, setle))  -> (xor (lshr (- %a), 63), 1)
     if(IsRHSZero)
       return getCompoundZeroComparisonInGPR(LHS, dl, ZeroCompare::LEZExt);
+
+    // The upper 32-bits of the register can't be undefined for this sequence.
+    LHS = signExtendInputIfNeeded(LHS);
+    RHS = signExtendInputIfNeeded(RHS);
     SDValue Sub =
-      SDValue(CurDAG->getMachineNode(PPC::SUBF, dl, MVT::i32, LHS, RHS), 0);
+      SDValue(CurDAG->getMachineNode(PPC::SUBF8, dl, MVT::i64, LHS, RHS), 0);
     SDValue Shift =
-      SDValue(CurDAG->getMachineNode(PPC::RLDICL_32, dl, MVT::i32, Sub,
+      SDValue(CurDAG->getMachineNode(PPC::RLDICL, dl, MVT::i64, Sub,
                                      getI64Imm(1, dl), getI64Imm(63, dl)), 0);
-    return SDValue(CurDAG->getMachineNode(PPC::XORI, dl,
-                   MVT::i32, Shift, getI32Imm(1, dl)), 0);
+    return
+      SDValue(CurDAG->getMachineNode(PPC::XORI8, dl,
+                                     MVT::i64, Shift, getI32Imm(1, dl)), 0);
   }
   }
 }
 
 /// Produces a sign-extended result of comparing two 32-bit values according to
 /// the passed condition code.
 SDValue PPCDAGToDAGISel::get32BitSExtCompare(SDValue LHS, SDValue RHS,
                                              ISD::CondCode CC,
                                              int64_t RHSValue, SDLoc dl) {
   bool IsRHSZero = RHSValue == 0;
-  bool IsRHSOne = RHSValue == 1;
-  bool IsRHSNegOne = RHSValue == -1LL;
   switch (CC) {
   default: return SDValue();
   case ISD::SETEQ: {
     // (sext (setcc %a, %b, seteq)) ->
     //   (ashr (shl (ctlz (xor %a, %b)), 58), 63)
     // (sext (setcc %a, 0, seteq)) ->
     //   (ashr (shl (ctlz %a), 58), 63)
     SDValue CountInput = IsRHSZero ? LHS :
       SDValue(CurDAG->getMachineNode(PPC::XOR, dl, MVT::i32, LHS, RHS), 0);
     SDValue Cntlzw =
       SDValue(CurDAG->getMachineNode(PPC::CNTLZW, dl, MVT::i32, CountInput), 0);
-    SDValue SHLOps[] = { Cntlzw, getI32Imm(58, dl), getI32Imm(0, dl) };
-    SDValue Sldi =
-      SDValue(CurDAG->getMachineNode(PPC::RLDICR_32, dl, MVT::i32, SHLOps), 0);
-    return SDValue(CurDAG->getMachineNode(PPC::SRADI_32, dl, MVT::i32, Sldi,
-                                          getI32Imm(63, dl)), 0);
+    SDValue SHLOps[] = { Cntlzw, getI32Imm(27, dl),
+                         getI32Imm(5, dl), getI32Imm(31, dl) };
+    SDValue Slwi =
+      SDValue(CurDAG->getMachineNode(PPC::RLWINM, dl, MVT::i32, SHLOps), 0);
+    return SDValue(CurDAG->getMachineNode(PPC::NEG, dl, MVT::i32, Slwi), 0);
   }
   case ISD::SETNE: {
     // Bitwise xor the operands, count leading zeros, shift right by 5 bits and
     // flip the bit, finally take 2's complement.
     // (sext (setcc %a, %b, setne)) ->
     //   (neg (xor (lshr (ctlz (xor %a, %b)), 5), 1))
     // Same as above, but the first xor is not needed.
     // (sext (setcc %a, 0, setne)) ->
case ISD::SETGE: {
     ConstantSDNode *RHSConst = dyn_cast<ConstantSDNode>(RHS);
     IsRHSZero = RHSConst && RHSConst->isNullValue();
     LLVM_FALLTHROUGH;
   }
   case ISD::SETLE: {
     // (sext (setcc %a, %b, setge)) -> (add (lshr (sub %b, %a), 63), -1)
     // (sext (setcc %a, 0, setle))  -> (add (lshr (- %a), 63), -1)
     if (IsRHSZero)
       return getCompoundZeroComparisonInGPR(LHS, dl, ZeroCompare::LESExt);
+
+    // The upper 32-bits of the register can't be undefined for this sequence.
+    LHS = signExtendInputIfNeeded(LHS);
+    RHS = signExtendInputIfNeeded(RHS);
     SDValue SUBFNode =
-      SDValue(CurDAG->getMachineNode(PPC::SUBF, dl, MVT::i32, MVT::Glue,
+      SDValue(CurDAG->getMachineNode(PPC::SUBF8, dl, MVT::i64, MVT::Glue,
                                      LHS, RHS), 0);
     SDValue Srdi =
-      SDValue(CurDAG->getMachineNode(PPC::RLDICL_32, dl, MVT::i32,
+      SDValue(CurDAG->getMachineNode(PPC::RLDICL, dl, MVT::i64,
                                      SUBFNode, getI64Imm(1, dl),
                                      getI64Imm(63, dl)), 0);
-    return SDValue(CurDAG->getMachineNode(PPC::ADDI, dl, MVT::i32, Srdi,
+    return SDValue(CurDAG->getMachineNode(PPC::ADDI8, dl, MVT::i64, Srdi,
                                           getI32Imm(-1, dl)), 0);
   }
   }
 }
 
 /// Produces a zero-extended result of comparing two 64-bit values according to
 /// the passed condition code.
 SDValue PPCDAGToDAGISel::get64BitZExtCompare(SDValue LHS, SDValue RHS,
SDValue PPCDAGToDAGISel::getSETCCInGPR(SDValue Compare,
   if (InputVT != MVT::i32 && InputVT != MVT::i64)
     return SDValue();
 
   if (ConvOpts == SetccInGPROpts::ZExtInvert ||
       ConvOpts == SetccInGPROpts::SExtInvert)
     CC = ISD::getSetCCInverse(CC, true);
 
   bool Inputs32Bit = InputVT == MVT::i32;
-  if (ISD::isSignedIntSetCC(CC) && Inputs32Bit) {
-    LHS = signExtendInputIfNeeded(LHS);
-    RHS = signExtendInputIfNeeded(RHS);
-  } else if (ISD::isUnsignedIntSetCC(CC) && Inputs32Bit) {
-    LHS = zeroExtendInputIfNeeded(LHS);
-    RHS = zeroExtendInputIfNeeded(RHS);
-  }
 
   SDLoc dl(Compare);
   ConstantSDNode *RHSConst = dyn_cast<ConstantSDNode>(RHS);
   int64_t RHSValue = RHSConst ? RHSConst->getSExtValue() : INT64_MAX;
   bool IsSext = ConvOpts == SetccInGPROpts::SExtOrig ||
     ConvOpts == SetccInGPROpts::SExtInvert;
 
   if (IsSext && Inputs32Bit)