Skip to content

Commit 3804663

Browse files
author
Elliot Colp
committed
Fix SystemZ hang caused by r279105
The change in r279105 causes an infinite loop in some cases, as it sets the upper bits of an AND mask constant, which DAGCombiner::SimplifyDemandedBits then unsets. This patch reverts that part of the behaviour, instead relying on .td peepholes to perform the transformation to NILL. I reapplied my original fix for the problem addressed by r279105 (unsetting the upper bits, which prevents a compiler abort for a different reason). Differential Revision: https://reviews.llvm.org/D23781 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@279515 91177308-0d34-0410-b5e6-96231b3b80d8
1 parent 5bf89a1 commit 3804663

File tree

2 files changed

+55
-29
lines changed

2 files changed

+55
-29
lines changed

lib/Target/SystemZ/SystemZISelLowering.cpp

Lines changed: 24 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -5095,21 +5095,21 @@ SDValue SystemZTargetLowering::combineSHIFTROT(
50955095
// value that has its last 6 bits set, we can safely remove the AND operation.
50965096
//
50975097
// If the AND operation doesn't have the last 6 bits set, we can't remove it
5098-
// entirely, but we can still truncate it to a 16-bit value with all other
5099-
// bits set. This will allow us to generate a NILL instead of a NILF for
5100-
// smaller code size.
5098+
// entirely, but we can still truncate it to a 16-bit value. This prevents
5099+
// us from ending up with a NILL with a signed operand, which will cause the
5100+
// instruction printer to abort.
51015101
SDValue N1 = N->getOperand(1);
51025102
if (N1.getOpcode() == ISD::AND) {
5103-
SDValue AndOp = N1->getOperand(0);
51045103
SDValue AndMaskOp = N1->getOperand(1);
51055104
auto *AndMask = dyn_cast<ConstantSDNode>(AndMaskOp);
51065105

51075106
// The AND mask is constant
51085107
if (AndMask) {
5109-
uint64_t AmtVal = AndMask->getZExtValue();
5110-
5108+
auto AmtVal = AndMask->getZExtValue();
5109+
51115110
// Bottom 6 bits are set
51125111
if ((AmtVal & 0x3f) == 0x3f) {
5112+
SDValue AndOp = N1->getOperand(0);
51135113

51145114
// This is the only use, so remove the node
51155115
if (N1.hasOneUse()) {
@@ -5129,30 +5129,25 @@ SDValue SystemZTargetLowering::combineSHIFTROT(
51295129
return Replace;
51305130
}
51315131

5132-
// We can't remove the AND, but we can use NILL here instead of NILF if we
5133-
// truncate the mask to 16 bits and set the remaining bits
5134-
} else {
5135-
unsigned BitWidth = AndMask->getAPIntValue().getBitWidth();
5136-
5137-
// All bits for the operand's size except the lower 16
5138-
uint64_t UpperBits = ((1ull << (uint64_t)BitWidth) - 1ull) &
5139-
0xffffffffffff0000ull;
5140-
5141-
if ((AmtVal & UpperBits) != UpperBits) {
5142-
auto NewMaskValue = (AmtVal & 0xffff) | UpperBits;
5143-
5144-
auto NewMask = DAG.getConstant(NewMaskValue,
5145-
SDLoc(AndMaskOp),
5146-
AndMaskOp.getValueType());
5147-
auto NewAnd = DAG.getNode(N1.getOpcode(), SDLoc(N1), N1.getValueType(),
5148-
AndOp, NewMask);
5149-
auto Replace = DAG.getNode(N->getOpcode(), SDLoc(N),
5150-
N->getValueType(0), N->getOperand(0),
5151-
NewAnd);
5152-
DCI.AddToWorklist(Replace.getNode());
5132+
// We can't remove the AND, but we can use NILL here (normally we would
5133+
// use NILF). Only keep the last 16 bits of the mask. The actual
5134+
// transformation will be handled by .td definitions.
5135+
} else if (AmtVal >> 16 != 0) {
5136+
SDValue AndOp = N1->getOperand(0);
51535137

5154-
return Replace;
5155-
}
5138+
auto NewMask = DAG.getConstant(AndMask->getZExtValue() & 0x0000ffff,
5139+
SDLoc(AndMaskOp),
5140+
AndMaskOp.getValueType());
5141+
5142+
auto NewAnd = DAG.getNode(N1.getOpcode(), SDLoc(N1), N1.getValueType(),
5143+
AndOp, NewMask);
5144+
5145+
SDValue Replace = DAG.getNode(N->getOpcode(), SDLoc(N),
5146+
N->getValueType(0), N->getOperand(0),
5147+
NewAnd);
5148+
DCI.AddToWorklist(Replace.getNode());
5149+
5150+
return Replace;
51565151
}
51575152
}
51585153
}

lib/Target/SystemZ/SystemZInstrInfo.td

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1834,6 +1834,37 @@ def : Pat<(sra (shl (i64 (anyext (i32 (z_select_ccmask 1, 0, imm32zx4:$valid,
18341834
def : Pat<(and (xor GR64:$x, (i64 -1)), GR64:$y),
18351835
(XGR GR64:$y, (NGR GR64:$y, GR64:$x))>;
18361836

1837+
// Shift/rotate instructions only use the last 6 bits of the second operand
1838+
// register, so we can safely use NILL (16 fewer bits than NILF) to only AND the
1839+
// last 16 bits.
1840+
// Complexity is added so that we match this before we match NILF on the AND
1841+
// operation alone.
1842+
let AddedComplexity = 4 in {
1843+
def : Pat<(shl GR32:$val, (and GR32:$shift, uimm32:$imm)),
1844+
(SLL GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1845+
1846+
def : Pat<(sra GR32:$val, (and GR32:$shift, uimm32:$imm)),
1847+
(SRA GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1848+
1849+
def : Pat<(srl GR32:$val, (and GR32:$shift, uimm32:$imm)),
1850+
(SRL GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1851+
1852+
def : Pat<(shl GR64:$val, (and GR32:$shift, uimm32:$imm)),
1853+
(SLLG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1854+
1855+
def : Pat<(sra GR64:$val, (and GR32:$shift, uimm32:$imm)),
1856+
(SRAG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1857+
1858+
def : Pat<(srl GR64:$val, (and GR32:$shift, uimm32:$imm)),
1859+
(SRLG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1860+
1861+
def : Pat<(rotl GR32:$val, (and GR32:$shift, uimm32:$imm)),
1862+
(RLL GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1863+
1864+
def : Pat<(rotl GR64:$val, (and GR32:$shift, uimm32:$imm)),
1865+
(RLLG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
1866+
}
1867+
18371868
// Peepholes for turning scalar operations into block operations.
18381869
defm : BlockLoadStore<anyextloadi8, i32, MVCSequence, NCSequence, OCSequence,
18391870
XCSequence, 1>;

0 commit comments

Comments
 (0)