-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[GlobalIsel] Visit ICmp #105991
[GlobalIsel] Visit ICmp #105991
Changes from 6 commits
a6f9276
eb751b6
39e49c7
ef71a60
077a9db
328c62b
286061c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -593,5 +593,31 @@ bool isGuaranteedNotToBeUndef(Register Reg, const MachineRegisterInfo &MRI, | |
/// estimate of the type. | ||
Type *getTypeForLLT(LLT Ty, LLVMContext &C); | ||
|
||
enum class GIConstantKind { Scalar, FixedVector, ScalableVector }; | ||
|
||
/// An integer-like constant. | ||
class GIConstant { | ||
GIConstantKind Kind; | ||
SmallVector<APInt> Values; | ||
APInt Value; | ||
|
||
public: | ||
GIConstant(ArrayRef<APInt> Values) | ||
: Kind(GIConstantKind::FixedVector), Values(Values) {}; | ||
GIConstant(const APInt &Value, GIConstantKind Kind) | ||
: Kind(Kind), Value(Value) {}; | ||
|
||
GIConstantKind getKind() const { return Kind; } | ||
|
||
APInt getScalarValue() const; | ||
|
||
static std::optional<GIConstant> getConstant(Register Const, | ||
const MachineRegisterInfo &MRI); | ||
}; | ||
|
||
/// Return true if the given value is known to be non-zero when defined. | ||
bool isKnownNonZero(Register Reg, const MachineRegisterInfo &MRI, | ||
GISelKnownBits *KB, unsigned Depth = 0); | ||
|
||
Comment on lines
+598
to
+621
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have similar interfaces like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I'm aware, I said
which returns an APInt. So if we add GIConstant now we have even more ways to do similar things. I'm not saying your approach is necessarily wrong here, but taking a step back maybe this change should be a separate effort to unify our utilities better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were limit to splat vectors before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with a family of tests for various kinds of constnesses, but I would argue that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Perhaps you're right, all the more reason to do it in a separate change where we can think about the design with more care. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The design is minimal, it has uses, and getting rid of the splat vector variants will take a lot of energy. I would prefer to continue with this PR. In a separate change, it would be dead code. |
||
} // End namespace llvm. | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1007,9 +1007,6 @@ def double_icmp_zero_or_combine: GICombineRule< | |
(G_ICMP $root, $p, $ordst, 0)) | ||
>; | ||
|
||
def double_icmp_zero_and_or_combine : GICombineGroup<[double_icmp_zero_and_combine, | ||
double_icmp_zero_or_combine]>; | ||
|
||
def and_or_disjoint_mask : GICombineRule< | ||
(defs root:$root, build_fn_matchinfo:$info), | ||
(match (wip_match_opcode G_AND):$root, | ||
|
@@ -1884,6 +1881,28 @@ def cast_combines: GICombineGroup<[ | |
buildvector_of_truncate | ||
]>; | ||
|
||
def visit_icmp : GICombineRule< | ||
tschuett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(defs root:$root, build_fn_matchinfo:$matchinfo), | ||
(match (G_ICMP $root, $pred, $lhs, $rhs):$cmp, | ||
[{ return Helper.visitICmp(*${cmp}, ${matchinfo}); }]), | ||
(apply [{ Helper.applyBuildFn(*${cmp}, ${matchinfo}); }])>; | ||
|
||
def icmp_of_zero : GICombineRule< | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The icmp x, 0 combine has precise MIR pattern and is not part of the visit function. I would never ever do that. |
||
(defs root:$root, build_fn_matchinfo:$matchinfo), | ||
(match (G_CONSTANT $zero, 0), | ||
(G_ICMP $root, $pred, $lhs, $zero):$cmp, | ||
[{ return Helper.matchCmpOfZero(*${cmp}, ${matchinfo}); }]), | ||
(apply [{ Helper.applyBuildFn(*${cmp}, ${matchinfo}); }])>; | ||
|
||
def icmp_combines: GICombineGroup<[ | ||
visit_icmp, | ||
icmp_of_zero, | ||
icmp_to_true_false_known_bits, | ||
icmp_to_lhs_known_bits, | ||
double_icmp_zero_and_combine, | ||
double_icmp_zero_or_combine, | ||
redundant_binop_in_equality | ||
]>; | ||
|
||
// FIXME: These should use the custom predicate feature once it lands. | ||
def undef_combines : GICombineGroup<[undef_to_fp_zero, undef_to_int_zero, | ||
|
@@ -1917,7 +1936,7 @@ def const_combines : GICombineGroup<[constant_fold_fp_ops, const_ptradd_to_i2p, | |
|
||
def known_bits_simplifications : GICombineGroup<[ | ||
redundant_and, redundant_sext_inreg, redundant_or, urem_pow2_to_mask, | ||
zext_trunc_fold, icmp_to_true_false_known_bits, icmp_to_lhs_known_bits, | ||
zext_trunc_fold, | ||
sext_inreg_to_zext_inreg]>; | ||
|
||
def width_reduction_combines : GICombineGroup<[reduce_shl_of_extend, | ||
|
@@ -1944,8 +1963,8 @@ def constant_fold_binops : GICombineGroup<[constant_fold_binop, | |
|
||
def prefer_sign_combines : GICombineGroup<[nneg_zext]>; | ||
|
||
def all_combines : GICombineGroup<[integer_reassoc_combines, trivial_combines, | ||
vector_ops_combines, freeze_combines, cast_combines, | ||
def all_combines : GICombineGroup<[icmp_combines, integer_reassoc_combines, | ||
trivial_combines, vector_ops_combines, freeze_combines, cast_combines, | ||
insert_vec_elt_combines, extract_vec_elt_combines, combines_for_extload, | ||
combine_extracted_vector_load, | ||
undef_combines, identity_combines, phi_combines, | ||
|
@@ -1964,9 +1983,9 @@ def all_combines : GICombineGroup<[integer_reassoc_combines, trivial_combines, | |
constant_fold_cast_op, fabs_fneg_fold, | ||
intdiv_combines, mulh_combines, redundant_neg_operands, | ||
and_or_disjoint_mask, fma_combines, fold_binop_into_select, | ||
sub_add_reg, select_to_minmax, redundant_binop_in_equality, | ||
sub_add_reg, select_to_minmax, | ||
fsub_to_fneg, commute_constant_to_rhs, match_ands, match_ors, | ||
combine_concat_vector, double_icmp_zero_and_or_combine, match_addos, | ||
combine_concat_vector, match_addos, | ||
sext_trunc, zext_trunc, prefer_sign_combines, combine_shuffle_concat]>; | ||
|
||
// A combine group used to for prelegalizer combiners at -O0. The combines in | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
//===- CombinerHelperCompares.cpp------------------------------------------===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This file implements CombinerHelper for G_ICMP. | ||
// | ||
//===----------------------------------------------------------------------===// | ||
#include "llvm/CodeGen/GlobalISel/CombinerHelper.h" | ||
#include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h" | ||
#include "llvm/CodeGen/GlobalISel/LegalizerHelper.h" | ||
#include "llvm/CodeGen/GlobalISel/LegalizerInfo.h" | ||
#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h" | ||
#include "llvm/CodeGen/GlobalISel/Utils.h" | ||
#include "llvm/CodeGen/LowLevelTypeUtils.h" | ||
#include "llvm/CodeGen/MachineInstr.h" | ||
#include "llvm/CodeGen/MachineOperand.h" | ||
#include "llvm/CodeGen/MachineRegisterInfo.h" | ||
#include "llvm/CodeGen/TargetOpcodes.h" | ||
#include "llvm/IR/Instructions.h" | ||
#include "llvm/Support/Casting.h" | ||
#include "llvm/Support/ErrorHandling.h" | ||
#include <cstdlib> | ||
|
||
#define DEBUG_TYPE "gi-combiner" | ||
|
||
using namespace llvm; | ||
|
||
bool CombinerHelper::constantFoldICmp(const GICmp &ICmp, | ||
const GIConstant &LHSCst, | ||
const GIConstant &RHSCst, | ||
BuildFnTy &MatchInfo) { | ||
if (LHSCst.getKind() != GIConstantKind::Scalar) | ||
return false; | ||
|
||
Register Dst = ICmp.getReg(0); | ||
LLT DstTy = MRI.getType(Dst); | ||
|
||
if (!isConstantLegalOrBeforeLegalizer(DstTy)) | ||
return false; | ||
|
||
CmpInst::Predicate Pred = ICmp.getCond(); | ||
APInt LHS = LHSCst.getScalarValue(); | ||
APInt RHS = RHSCst.getScalarValue(); | ||
|
||
bool Result = ICmpInst::compare(LHS, RHS, Pred); | ||
|
||
MatchInfo = [=](MachineIRBuilder &B) { | ||
if (Result) | ||
B.buildConstant(Dst, getICmpTrueVal(getTargetLowering(), | ||
/*IsVector=*/DstTy.isVector(), | ||
/*IsFP=*/false)); | ||
else | ||
B.buildConstant(Dst, 0); | ||
}; | ||
|
||
return true; | ||
} | ||
|
||
bool CombinerHelper::visitICmp(const MachineInstr &MI, BuildFnTy &MatchInfo) { | ||
const GICmp *Cmp = cast<GICmp>(&MI); | ||
|
||
Register Dst = Cmp->getReg(0); | ||
LLT DstTy = MRI.getType(Dst); | ||
Register LHS = Cmp->getLHSReg(); | ||
Register RHS = Cmp->getRHSReg(); | ||
|
||
CmpInst::Predicate Pred = Cmp->getCond(); | ||
assert(CmpInst::isIntPredicate(Pred) && "Not an integer compare!"); | ||
if (auto CLHS = GIConstant::getConstant(LHS, MRI)) { | ||
if (auto CRHS = GIConstant::getConstant(RHS, MRI)) | ||
return constantFoldICmp(*Cmp, *CLHS, *CRHS, MatchInfo); | ||
|
||
// If we have a constant, make sure it is on the RHS. | ||
std::swap(LHS, RHS); | ||
Pred = CmpInst::getSwappedPredicate(Pred); | ||
tschuett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
MatchInfo = [=](MachineIRBuilder &B) { B.buildICmp(Pred, Dst, LHS, RHS); }; | ||
return true; | ||
} | ||
|
||
MachineInstr *MIRHS = MRI.getVRegDef(RHS); | ||
|
||
// For EQ and NE, we can always pick a value for the undef to make the | ||
// predicate pass or fail, so we can return undef. | ||
// Matches behavior in llvm::ConstantFoldCompareInstruction. | ||
if (isa<GImplicitDef>(MIRHS) && ICmpInst::isEquality(Pred) && | ||
isLegalOrBeforeLegalizer({TargetOpcode::G_IMPLICIT_DEF, {DstTy}})) { | ||
MatchInfo = [=](MachineIRBuilder &B) { B.buildUndef(Dst); }; | ||
return true; | ||
} | ||
tschuett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// icmp X, X -> true/false | ||
// icmp X, undef -> true/false because undef could be X. | ||
if ((LHS == RHS || isa<GImplicitDef>(MIRHS)) && | ||
isConstantLegalOrBeforeLegalizer(DstTy)) { | ||
MatchInfo = [=](MachineIRBuilder &B) { | ||
if (CmpInst::isTrueWhenEqual(Pred)) | ||
B.buildConstant(Dst, getICmpTrueVal(getTargetLowering(), | ||
/*IsVector=*/DstTy.isVector(), | ||
/*IsFP=*/false)); | ||
else | ||
B.buildConstant(Dst, 0); | ||
}; | ||
return true; | ||
} | ||
tschuett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return false; | ||
} | ||
|
||
bool CombinerHelper::matchCmpOfZero(const MachineInstr &MI, | ||
BuildFnTy &MatchInfo) { | ||
const GICmp *Cmp = cast<GICmp>(&MI); | ||
|
||
Register Dst = Cmp->getReg(0); | ||
LLT DstTy = MRI.getType(Dst); | ||
Register LHS = Cmp->getLHSReg(); | ||
CmpInst::Predicate Pred = Cmp->getCond(); | ||
|
||
if (!isConstantLegalOrBeforeLegalizer(DstTy)) | ||
return false; | ||
|
||
std::optional<bool> Result; | ||
|
||
switch (Pred) { | ||
default: | ||
llvm_unreachable("Unkonwn ICmp predicate!"); | ||
case ICmpInst::ICMP_ULT: | ||
Result = false; | ||
break; | ||
case ICmpInst::ICMP_UGE: | ||
Result = true; | ||
break; | ||
case ICmpInst::ICMP_EQ: | ||
case ICmpInst::ICMP_ULE: | ||
if (isKnownNonZero(LHS, MRI, KB)) | ||
Result = false; | ||
break; | ||
case ICmpInst::ICMP_NE: | ||
case ICmpInst::ICMP_UGT: | ||
if (isKnownNonZero(LHS, MRI, KB)) | ||
Result = true; | ||
break; | ||
case ICmpInst::ICMP_SLT: { | ||
KnownBits LHSKnown = KB->getKnownBits(LHS); | ||
if (LHSKnown.isNegative()) | ||
Result = true; | ||
if (LHSKnown.isNonNegative()) | ||
Result = false; | ||
break; | ||
} | ||
case ICmpInst::ICMP_SLE: { | ||
KnownBits LHSKnown = KB->getKnownBits(LHS); | ||
if (LHSKnown.isNegative()) | ||
Result = true; | ||
if (LHSKnown.isNonNegative() && isKnownNonZero(LHS, MRI, KB)) | ||
Result = false; | ||
break; | ||
} | ||
case ICmpInst::ICMP_SGE: { | ||
KnownBits LHSKnown = KB->getKnownBits(LHS); | ||
if (LHSKnown.isNegative()) | ||
Result = false; | ||
if (LHSKnown.isNonNegative()) | ||
Result = true; | ||
break; | ||
} | ||
case ICmpInst::ICMP_SGT: { | ||
KnownBits LHSKnown = KB->getKnownBits(LHS); | ||
if (LHSKnown.isNegative()) | ||
Result = false; | ||
if (LHSKnown.isNonNegative() && isKnownNonZero(LHS, MRI, KB)) | ||
Result = true; | ||
break; | ||
} | ||
} | ||
|
||
if (!Result) | ||
return false; | ||
|
||
MatchInfo = [=](MachineIRBuilder &B) { | ||
if (*Result) | ||
B.buildConstant(Dst, getICmpTrueVal(getTargetLowering(), | ||
/*IsVector=*/DstTy.isVector(), | ||
/*IsFP=*/false)); | ||
else | ||
B.buildConstant(Dst, 0); | ||
}; | ||
|
||
return true; | ||
} |
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 extract the infrastructure bits out 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.
GSplatVector is already upstream.
GZextOrSextOp
has probably a lower market share.