Skip to content
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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
#include "llvm/CodeGen/GlobalISel/Utils.h"
#include "llvm/CodeGen/Register.h"
#include "llvm/CodeGenTypes/LowLevelType.h"
#include "llvm/IR/InstrTypes.h"
Expand Down Expand Up @@ -299,6 +300,10 @@ class CombinerHelper {
/// $whatever = COPY $addr
bool tryCombineMemCpyFamily(MachineInstr &MI, unsigned MaxLen = 0);

bool matchICmp(const MachineInstr &MI, BuildFnTy &MatchInfo);
/// Try hard to fold icmp with zero RHS because this is a common case.
bool matchCmpOfZero(const MachineInstr &MI, BuildFnTy &MatchInfo);

bool matchPtrAddImmedChain(MachineInstr &MI, PtrAddChain &MatchInfo);
void applyPtrAddImmedChain(MachineInstr &MI, PtrAddChain &MatchInfo);

Expand Down Expand Up @@ -1017,6 +1022,9 @@ class CombinerHelper {
bool tryFoldLogicOfFCmps(GLogicalBinOp *Logic, BuildFnTy &MatchInfo);

bool isCastFree(unsigned Opcode, LLT ToTy, LLT FromTy) const;

bool constantFoldICmp(const GICmp &ICmp, const GIConstant &LHS,
const GIConstant &RHS, BuildFnTy &MatchInfo);
};
} // namespace llvm

Expand Down
24 changes: 24 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,30 @@ class GExtOrTruncOp : public GCastOp {
};
};

/// Represents a splat vector.
class GSplatVector : public GenericMachineInstr {
public:
Register getValueReg() const { return getOperand(1).getReg(); }

static bool classof(const MachineInstr *MI) {
return MI->getOpcode() == TargetOpcode::G_SPLAT_VECTOR;
};
};

/// Represents an integer-like extending operation.
class GZextOrSextOp : public GCastOp {
public:
static bool classof(const MachineInstr *MI) {
switch (MI->getOpcode()) {
case TargetOpcode::G_SEXT:
case TargetOpcode::G_ZEXT:
return true;
default:
return false;
}
};
};

Comment on lines +953 to +976
Copy link
Contributor

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

Copy link
Author

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.

} // namespace llvm

#endif // LLVM_CODEGEN_GLOBALISEL_GENERICMACHINEINSTRS_H
26 changes: 26 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/Utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have similar interfaces like isConstantOrConstantVector and friends. Can we extend those or unify in some more consistent way rather than have 2 different ways of querying constant values?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that isConstantOrConstantVectorstarts with is. It is a test. GIConstant::getConstant gives you a constant. It is a vector of APInt s. We can add arbitrary arithmetic. GIConstant + GIConstant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm aware, I said isConstantOrConstantVector and friends. One of those friends is this:

std::optional<APInt>
llvm::isConstantOrConstantSplatVector(

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were limit to splat vectors before. GIConstant does not have this restriction.

Copy link
Author

Choose a reason for hiding this comment

The 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 GIConstant is powerful enough to become the constant. It is a class. We can add arbitrary member functions for tests and arithmetic. Constants can be scalar, fixed-, and scalable-vectors.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 GIConstant is powerful enough to become the constant. It is a class. We can add arbitrary member functions for tests and arithmetic. Constants can be scalar, fixed-, and scalable-vectors.

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.

Copy link
Author

Choose a reason for hiding this comment

The 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
35 changes: 27 additions & 8 deletions llvm/include/llvm/Target/GlobalISel/Combine.td
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1884,6 +1881,28 @@ def cast_combines: GICombineGroup<[
buildvector_of_truncate
]>;

def prepare_icmp : GICombineRule<
(defs root:$root, build_fn_matchinfo:$matchinfo),
(match (G_ICMP $root, $pred, $lhs, $rhs):$cmp,
[{ return Helper.matchICmp(*${cmp}, ${matchinfo}); }]),
(apply [{ Helper.applyBuildFn(*${cmp}, ${matchinfo}); }])>;

def icmp_of_zero : GICombineRule<
Copy link
Author

Choose a reason for hiding this comment

The 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<[
prepare_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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/CodeGen/GlobalISel/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ add_llvm_component_library(LLVMGlobalISel
Combiner.cpp
CombinerHelper.cpp
CombinerHelperCasts.cpp
CombinerHelperCompares.cpp
CombinerHelperVectorOps.cpp
GIMatchTableExecutor.cpp
GISelChangeObserver.cpp
Expand Down
167 changes: 167 additions & 0 deletions llvm/lib/CodeGen/GlobalISel/CombinerHelperCompares.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
//===- 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::matchICmp(const MachineInstr &MI, BuildFnTy &MatchInfo) {
const GICmp *Cmp = cast<GICmp>(&MI);

Register Dst = Cmp->getReg(0);
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);

MatchInfo = [=](MachineIRBuilder &B) { B.buildICmp(Pred, Dst, LHS, RHS); };
return true;
}

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;
}
Loading
Loading