-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[SelectionDAG] Add preliminary plumbing for samesign
flag
#112354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SelectionDAG] Add preliminary plumbing for samesign
flag
#112354
Conversation
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-llvm-selectiondag Author: Antonio Frighetto (antoniofrighetto) ChangesExtend recently-added poison-generating IR flag to codegen as well. I think there are some other places in TargetLowering (e.g., Full diff: https://github.com/llvm/llvm-project/pull/112354.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index 639e9311977502..bad19173683101 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -382,6 +382,7 @@ struct SDNodeFlags {
bool NoSignedWrap : 1;
bool Exact : 1;
bool Disjoint : 1;
+ bool SameSign : 1;
bool NonNeg : 1;
bool NoNaNs : 1;
bool NoInfs : 1;
@@ -404,10 +405,10 @@ struct SDNodeFlags {
/// Default constructor turns off all optimization flags.
SDNodeFlags()
: NoUnsignedWrap(false), NoSignedWrap(false), Exact(false),
- Disjoint(false), NonNeg(false), NoNaNs(false), NoInfs(false),
- NoSignedZeros(false), AllowReciprocal(false), AllowContract(false),
- ApproximateFuncs(false), AllowReassociation(false), NoFPExcept(false),
- Unpredictable(false) {}
+ Disjoint(false), SameSign(false), NonNeg(false), NoNaNs(false),
+ NoInfs(false), NoSignedZeros(false), AllowReciprocal(false),
+ AllowContract(false), ApproximateFuncs(false),
+ AllowReassociation(false), NoFPExcept(false), Unpredictable(false) {}
/// Propagate the fast-math-flags from an IR FPMathOperator.
void copyFMF(const FPMathOperator &FPMO) {
@@ -425,6 +426,7 @@ struct SDNodeFlags {
void setNoSignedWrap(bool b) { NoSignedWrap = b; }
void setExact(bool b) { Exact = b; }
void setDisjoint(bool b) { Disjoint = b; }
+ void setSameSign(bool b) { SameSign = b; }
void setNonNeg(bool b) { NonNeg = b; }
void setNoNaNs(bool b) { NoNaNs = b; }
void setNoInfs(bool b) { NoInfs = b; }
@@ -441,6 +443,7 @@ struct SDNodeFlags {
bool hasNoSignedWrap() const { return NoSignedWrap; }
bool hasExact() const { return Exact; }
bool hasDisjoint() const { return Disjoint; }
+ bool hasSameSign() const { return SameSign; }
bool hasNonNeg() const { return NonNeg; }
bool hasNoNaNs() const { return NoNaNs; }
bool hasNoInfs() const { return NoInfs; }
@@ -473,6 +476,7 @@ struct SDNodeFlags {
NoSignedWrap &= Flags.NoSignedWrap;
Exact &= Flags.Exact;
Disjoint &= Flags.Disjoint;
+ SameSign &= Flags.SameSign;
NonNeg &= Flags.NonNeg;
NoNaNs &= Flags.NoNaNs;
NoInfs &= Flags.NoInfs;
@@ -1032,7 +1036,7 @@ END_TWO_BYTE_PACK()
SDNodeFlags Flags = getFlags();
return Flags.hasNoUnsignedWrap() || Flags.hasNoSignedWrap() ||
Flags.hasExact() || Flags.hasDisjoint() || Flags.hasNonNeg() ||
- Flags.hasNoNaNs() || Flags.hasNoInfs();
+ Flags.hasNoNaNs() || Flags.hasNoInfs() || Flags.hasSameSign();
}
void setCFIType(uint32_t Type) { CFIType = Type; }
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 805b8ecf009598..35ea9419f2dd1a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3640,6 +3640,10 @@ void SelectionDAGBuilder::visitICmp(const ICmpInst &I) {
SDValue Op2 = getValue(I.getOperand(1));
ISD::CondCode Opcode = getICmpCondCode(predicate);
+ SDNodeFlags Flags;
+ Flags.setSameSign(I.hasSameSign());
+ SelectionDAG::FlagInserter FlagsInserter(DAG, Flags);
+
auto &TLI = DAG.getTargetLoweringInfo();
EVT MemVT =
TLI.getMemValueType(DAG.getDataLayout(), I.getOperand(0)->getType());
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
index 56fc538172f9fc..24fc83b422147f 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
@@ -651,6 +651,9 @@ void SDNode::print_details(raw_ostream &OS, const SelectionDAG *G) const {
if (getFlags().hasDisjoint())
OS << " disjoint";
+ if (getFlags().hasSameSign())
+ OS << " samesign";
+
if (getFlags().hasNonNeg())
OS << " nneg";
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 40f030d7b936f7..43e6a43e820b5a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -1703,6 +1703,7 @@ bool TargetLowering::SimplifyDemandedBits(
case ISD::SETCC: {
SDValue Op0 = Op.getOperand(0);
SDValue Op1 = Op.getOperand(1);
+ SDNodeFlags Flags = Op.getNode()->getFlags();
ISD::CondCode CC = cast<CondCodeSDNode>(Op.getOperand(2))->get();
// If (1) we only need the sign-bit, (2) the setcc operands are the same
// width as the setcc result, and (3) the result of a setcc conforms to 0 or
@@ -1716,8 +1717,13 @@ bool TargetLowering::SimplifyDemandedBits(
// if we don't care about FP signed-zero. The use of SETLT with FP means
// that we don't care about NaNs.
if (CC == ISD::SETLT && Op1.getValueType().isInteger() &&
- (isNullConstant(Op1) || ISD::isBuildVectorAllZeros(Op1.getNode())))
+ (isNullConstant(Op1) || ISD::isBuildVectorAllZeros(Op1.getNode()))) {
+ if (Flags.hasSameSign()) {
+ Flags.setSameSign(false);
+ Op->setFlags(Flags);
+ }
return TLO.CombineTo(Op, Op0);
+ }
// TODO: Should we check for other forms of sign-bit comparisons?
// Examples: X <= -1, X >= 0
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed operator==. Should InstrEmitter propagate it?
Added, thanks! Extended it to MIRParser as well. |
samesign
flagsamesign
flag
@@ -3640,6 +3640,10 @@ void SelectionDAGBuilder::visitICmp(const ICmpInst &I) { | |||
SDValue Op2 = getValue(I.getOperand(1)); | |||
ISD::CondCode Opcode = getICmpCondCode(predicate); | |||
|
|||
SDNodeFlags Flags; | |||
Flags.setSameSign(I.hasSameSign()); | |||
SelectionDAG::FlagInserter FlagsInserter(DAG, Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be below the possible getPtrExtOrTrunc
below.
(isNullConstant(Op1) || ISD::isBuildVectorAllZeros(Op1.getNode()))) { | ||
if (Flags.hasSameSign()) { | ||
Flags.setSameSign(false); | ||
Op->setFlags(Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem necessary for the plumbing and its not clear its correct to propagate the rest of the flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to stop propagating the flag in SimplifyDemandedBits
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I misunderstood earlier.
I don't think we need to drop Op
flags here. We aren't simplifying Op0
or Op1
here, just replacing Op
with Op0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, dropped it.
llvm/lib/CodeGen/MIRPrinter.cpp
Outdated
@@ -837,6 +837,8 @@ void MIPrinter::print(const MachineInstr &MI) { | |||
OS << "disjoint "; | |||
if (MI.getFlag(MachineInstr::NoUSWrap)) | |||
OS << "nusw "; | |||
if (MI.getFlag(MachineInstr::SameSign)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should get mir print / parse test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing this via GISel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't cover the parsing, see test/CodeGen/MIR/X86/fastmath.mir as an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, thanks, think it should be on track now.
(isNullConstant(Op1) || ISD::isBuildVectorAllZeros(Op1.getNode()))) { | ||
if (Flags.hasSameSign()) { | ||
Flags.setSameSign(false); | ||
Op->setFlags(Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs test and is beyond infrastructure work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped it.
samesign CMP32rr killed %0, killed %1, implicit-def $eflags | ||
$al = SETCCr 2, implicit killed $eflags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be more sensible to have this on a G_MIR instruction as a sample
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the test to use generic MIR. The gMIR comes from GISel, stopping before legalization. Also rebased to main for the recent enum change.
8bcf219
to
7c49142
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I just reviewed the same thing in #113090
SDNodeFlags Flags; | ||
Flags.setSameSign(I.hasSameSign()); | ||
SelectionDAG::FlagInserter FlagsInserter(DAG, Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need update after #114061
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was committed so can do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still free to use FlagInserter, independently of the changes in #114061, this should be fine.
; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(s32) = G_ANYEXT [[ZEXT]](s8) | ||
; CHECK-NEXT: $w0 = COPY [[ANYEXT]](s32) | ||
; CHECK-NEXT: RET_ReallyLR implicit $w0 | ||
%res = icmp samesign ult i32 %a, %b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no test without samesign
.
; CHECK-NEXT: RET 0, implicit $al | ||
%tmp0:_(s32) = COPY $edi | ||
%tmp1:_(s32) = COPY $esi | ||
%tmp4:_(s8) = samesign G_ICMP intpred(ult), %tmp0(s32), %tmp1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again no test without samesign
.
If everything works well this time, you can rebase and limit the changes to SDAG. |
@@ -340,18 +340,18 @@ bool IRTranslator::translateCompare(const User &U, | |||
Register Op1 = getOrCreateVReg(*U.getOperand(1)); | |||
Register Res = getOrCreateVReg(U); | |||
CmpInst::Predicate Pred = CI->getPredicate(); | |||
uint32_t Flags = 0; | |||
if (CI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI
is never nullptr
, but it will disappear during rebase anyway.
7c49142
to
4f95fbd
Compare
samesign
flagsamesign
flag
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr32 = COPY $w1 | ||
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr32 = COPY $w0 | ||
; CHECK-NEXT: [[SUBSWrr:%[0-9]+]]:gpr32 = SUBSWrr [[COPY1]], [[COPY]], implicit-def $nzcv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arsenm Was trying to test DAG preserving of the flag (to test InstrEmitter), but it seems like the flag gets lost during legalization of the DAG (which boils down to a custom LowerSETCC for AArch64/X86). Shouldn't we want to preserve the flag there as well, or is it beyond patch scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of preserving the flag is diminishing the further back you go. I doubt anything after selection will have any use of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we still preserve the flag immediately before doing instruction selection (before the very last iteration of DAG combining)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't really care, it's easy enough to handle. I would agree preserving the flag in other contexts is beyond the scope of this patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm dropping the test then, and merging this, if you are still fine with everything else.
@@ -1267,7 +1267,7 @@ class MachineIRBuilder { | |||
/// \return a MachineInstrBuilder for the newly created instruction. | |||
MachineInstrBuilder buildICmp(CmpInst::Predicate Pred, const DstOp &Res, | |||
const SrcOp &Op0, const SrcOp &Op1, | |||
std::optional<unsigned> Flgs = std::nullopt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated typo fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the minor opportunity to fix the typo within this patch, will undo if you have a strong concern on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was probably me. I vote for fix.
SDNodeFlags Flags; | ||
Flags.setSameSign(I.hasSameSign()); | ||
SelectionDAG::FlagInserter FlagsInserter(DAG, Flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was committed so can do this
; CHECK-NEXT: [[CMHIv2i32_:%[0-9]+]]:fpr64 = CMHIv2i32 [[COPY]], [[COPY1]] | ||
; CHECK-NEXT: $d0 = COPY [[CMHIv2i32_]] | ||
; CHECK-NEXT: RET_ReallyLR implicit $d0 | ||
%res = icmp samesign ult <2 x i32> %a, %b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a test without samesign?
4f95fbd
to
54a58b1
Compare
Extend recently-added poison-generating IR flag to codegen as well.
54a58b1
to
19c8475
Compare
This PR caused an assertion error:
|
Patch attempting to fix the issue: #114582. |
This patch intersects poison-generating flags after CSE to fix assertion failure reported in #112354 (comment). Co-authored-by: Antonio Frighetto <[email protected]>
This patch intersects poison-generating flags after CSE to fix assertion failure reported in llvm#112354 (comment). Co-authored-by: Antonio Frighetto <[email protected]>
This patch intersects poison-generating flags after CSE to fix assertion failure reported in llvm#112354 (comment). Co-authored-by: Antonio Frighetto <[email protected]>
Extend recently-added poison-generating IR flag to codegen as well.
I think there are some other places in TargetLowering (e.g.,
LegalizeSetCCCondCode
) where the flag should be cleared, welcoming directions (also maybe besidesSDNodeFlags
, it can be added toMIFlag
directly here).