Skip to content

Commit

Permalink
Delete CompareOp::kIs{,Not}
Browse files Browse the repository at this point in the history 
Summary:
Currently these are unconditionally simplified down to
`PrimitiveCompare +
PrimitiveBoxBool`, but they still need to be handled
in the HIR and LIR "just in case".  We can avoid all this unnecessary code if we
generate them as the primitive ops to begin with in the HIR builder.

Reviewed By: swtaarrs

Differential Revision: D47294128

fbshipit-source-id: 151cf91609f57de54e5217b0c24c565e0959999e
  • Loading branch information
@facebook-github-bot
Alex Malyshev authored and facebook-github-bot committed Jul 13, 2023
1 parent 0a417ce commit e7d9af0
Show file tree
Hide file tree
Showing 12 changed files with 116 additions and 211 deletions.
7 changes: 5 additions & 2 deletions Jit/hir/builder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2100,9 +2100,12 @@ void HIRBuilder::emitIsOp(TranslationContext& tc, int oparg) {
auto& stack = tc.frame.stack;
Register* right = stack.pop();
Register* left = stack.pop();
Register* unboxed_result = temps_.AllocateStack();
Register* result = temps_.AllocateStack();
CompareOp op = oparg == 0 ? CompareOp::kIs : CompareOp::kIsNot;
tc.emit<Compare>(result, op, left, right, tc.frame);
auto op =
oparg == 0 ? PrimitiveCompareOp::kEqual : PrimitiveCompareOp::kNotEqual;
tc.emit<PrimitiveCompare>(unboxed_result, op, left, right);
tc.emit<PrimitiveBoxBool>(result, unboxed_result);
stack.push(result);
}

Expand Down
12 changes: 3 additions & 9 deletions Jit/hir/hir.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,6 @@ bool Instr::isReplayable() const {
case Opcode::kWaitHandleLoadWaiter: {
return true;
}
case Opcode::kCompare: {
auto op = static_cast<const Compare*>(this)->op();
return op == CompareOp::kIs || op == CompareOp::kIsNot;
}
case Opcode::kCompareBool: {
auto op = static_cast<const CompareBool*>(this)->op();
return op == CompareOp::kIs || op == CompareOp::kIsNot;
}
case Opcode::kBatchDecref:
case Opcode::kBeginInlinedFunction:
case Opcode::kBinaryOp:
Expand All @@ -197,9 +189,11 @@ bool Instr::isReplayable() const {
case Opcode::kCallMethod:
case Opcode::kCallStatic:
case Opcode::kCallStaticRetVoid:
case Opcode::kCompare:
case Opcode::kCompareBool:
case Opcode::kCondBranch:
case Opcode::kCondBranchIterNotDone:
case Opcode::kCondBranchCheckType:
case Opcode::kCondBranchIterNotDone:
case Opcode::kCopyDictWithoutKeys:
case Opcode::kDecref:
case Opcode::kDeleteAttr:
Expand Down
33 changes: 16 additions & 17 deletions Jit/hir/hir.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2186,23 +2186,22 @@ class INSTR_CLASS(PrimitiveUnaryOp, (TPrimitive), HasOutput, Operands<1>) {
PrimitiveUnaryOpKind op_;
};

#define FOREACH_COMPARE_OP(V) \
/* Begin rich comparison opcodes. */ \
V(LessThan) \
V(LessThanEqual) \
V(Equal) \
V(NotEqual) \
V(GreaterThan) \
V(GreaterThanEqual) \
/* End rich comparison opcodes. */ \
V(In) \
V(NotIn) \
V(Is) \
V(IsNot) \
V(ExcMatch) \
V(GreaterThanUnsigned) \
V(GreaterThanEqualUnsigned) \
V(LessThanUnsigned) \
#define FOREACH_COMPARE_OP(V) \
/* Begin rich comparison opcodes. */ \
V(LessThan) \
V(LessThanEqual) \
V(Equal) \
V(NotEqual) \
V(GreaterThan) \
V(GreaterThanEqual) \
/* End rich comparison opcodes. */ \
V(In) \
V(NotIn) \
/* Note: Is and IsNot are handled by PrimitiveCompare. */ \
V(ExcMatch) \
V(GreaterThanUnsigned) \
V(GreaterThanEqualUnsigned) \
V(LessThanUnsigned) \
V(LessThanEqualUnsigned)

enum class CompareOp {
Expand Down
13 changes: 0 additions & 13 deletions Jit/hir/optimization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,6 @@ std::unique_ptr<Pass> PassRegistry::MakePass(const std::string& name) {
Instr* DynamicComparisonElimination::ReplaceCompare(
Compare* compare,
IsTruthy* truthy) {
// For is/is not we can use CompareInt:
// $truthy = CompareInt<Eq> $x $y
// CondBranch<x, y> $truthy
// For other comparisons we can use ComapreBool.
if (compare->op() == CompareOp::kIs || compare->op() == CompareOp::kIsNot) {
return PrimitiveCompare::create(
truthy->GetOutput(),
(compare->op() == CompareOp::kIs) ? PrimitiveCompareOp::kEqual
: PrimitiveCompareOp::kNotEqual,
compare->GetOperand(0),
compare->GetOperand(1));
}

return CompareBool::create(
truthy->GetOutput(),
compare->op(),
Expand Down
28 changes: 3 additions & 25 deletions Jit/hir/simplify.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,31 +310,6 @@ Register* simplifyCompare(Env& env, const Compare* instr) {
Register* left = instr->GetOperand(0);
Register* right = instr->GetOperand(1);
CompareOp op = instr->op();
if (op == CompareOp::kIs || op == CompareOp::kIsNot) {
Type left_t = left->type();
Type right_t = right->type();
if (!left_t.couldBe(right_t)) {
env.emit<UseType>(left, left_t);
env.emit<UseType>(right, right_t);
return env.emit<LoadConst>(
Type::fromObject(op == CompareOp::kIs ? Py_False : Py_True));
}
PyObject* left_t_obj = left_t.asObject();
PyObject* right_t_obj = right_t.asObject();
if (left_t_obj != nullptr && right_t_obj != nullptr) {
env.emit<UseType>(left, left_t);
env.emit<UseType>(right, right_t);
bool same_obj = left_t_obj == right_t_obj;
bool truthy = (op == CompareOp::kIs) == same_obj;
return env.emit<LoadConst>(Type::fromObject(truthy ? Py_True : Py_False));
}
auto cbool = env.emit<PrimitiveCompare>(
instr->op() == CompareOp::kIs ? PrimitiveCompareOp::kEqual
: PrimitiveCompareOp::kNotEqual,
instr->left(),
instr->right());
return env.emit<PrimitiveBoxBool>(cbool);
}
if (left->isA(TNoneType) && right->isA(TNoneType)) {
if (op == CompareOp::kEqual || op == CompareOp::kNotEqual) {
env.emit<UseType>(left, TNoneType);
Expand Down Expand Up @@ -669,6 +644,9 @@ Register* simplifyPrimitiveCompare(Env& env, const PrimitiveCompare* instr) {
return env.emit<LoadConst>(Type::fromCBool(
instr->op() == PrimitiveCompareOp::kNotEqual ? !value : value));
};
if (!left->type().couldBe(right->type())) {
return do_cbool(false);
}
if (left->type().hasIntSpec() && right->type().hasIntSpec()) {
return do_cbool(left->type().intSpec() == right->type().intSpec());
}
Expand Down
16 changes: 0 additions & 16 deletions Jit/jit_rt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1782,22 +1782,6 @@ int JITRT_RichCompareBool(PyObject* v, PyObject* w, int op) {
return PyObject_IsTrue(res);
}

PyObject* JITRT_CompareIs(PyObject* left, PyObject* right, int op) {
if (op == static_cast<int>(jit::hir::CompareOp::kIs)) {
if (left == right) {
Py_RETURN_TRUE;
}
Py_RETURN_FALSE;
}
if (op == static_cast<int>(jit::hir::CompareOp::kIsNot)) {
if (left != right) {
Py_RETURN_TRUE;
}
Py_RETURN_FALSE;
}
JIT_ABORT("bad comparison op %d", op);
}

/* perform a batch decref to the objects in args */
void JITRT_BatchDecref(PyObject** args, int nargs) {
for (int i = 0; i < nargs; i++) {
Expand Down
5 changes: 0 additions & 5 deletions Jit/jit_rt.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,11 +521,6 @@ int JITRT_NotContainsBool(PyObject* w, PyObject* v);

int JITRT_RichCompareBool(PyObject* v, PyObject* w, int op);

/* Check if `left is right` (op == CompareOp::kIs) or `left is not right` (op
* == CompareOp::kIsNot), returning Py_True if the operation is true and
* Py_False if the operation is false. It will never return nullptr. */
PyObject* JITRT_CompareIs(PyObject* left, PyObject* right, int op);

/* perform a batch decref to the objects in args */
void JITRT_BatchDecref(PyObject** args, int nargs);

Expand Down
33 changes: 0 additions & 33 deletions Jit/lir/generator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1500,18 +1500,6 @@ LIRGenerator::TranslatedBlock LIRGenerator::TranslateOneBasicBlock(
instr->left());
break;
}
if (instr->op() == CompareOp::kIs || instr->op() == CompareOp::kIsNot) {
// This case should generally not happen, since Compare<Is> and
// Compare<IsNot> can be rewritten into PrimitiveCompare. We keep it
// around because optimization passes should not impact correctness.
bbb.AppendCall(
instr->dst(),
JITRT_CompareIs,
instr->right(),
instr->left(),
static_cast<int>(instr->op()));
break;
}
int op = static_cast<int>(instr->op());
JIT_CHECK(op >= Py_LT, "invalid compare op %d", op);
JIT_CHECK(op <= Py_GE, "invalid compare op %d", op);
Expand Down Expand Up @@ -1611,21 +1599,6 @@ LIRGenerator::TranslatedBlock LIRGenerator::TranslateOneBasicBlock(
instr->right(),
static_cast<int>(instr->op()));
} else {
if (instr->op() == CompareOp::kIs ||
instr->op() == CompareOp::kIsNot) {
// This case should generally not happen, since CompareBool<Is> and
// CompareBool<IsNot> can be rewritten into PrimitiveCompare. We
// keep it around because optimization passes should not impact
// correctness.
Instruction* compare_result = bbb.appendInstr(
Instruction::kEqual,
OutVReg{OperandBase::k8bit},
instr->left(),
instr->right());
bbb.appendInstr(
instr->GetOutput(), Instruction::kZext, compare_result);
break;
}
bbb.AppendCall(
instr->dst(),
JITRT_RichCompareBool,
Expand Down Expand Up @@ -2797,12 +2770,6 @@ LIRGenerator::TranslatedBlock LIRGenerator::TranslateOneBasicBlock(
break;
}
case Opcode::kCompare: {
auto op = static_cast<const Compare&>(i).op();
if (op == CompareOp::kIs || op == CompareOp::kIsNot) {
// These are implemented using pointer equality and cannot
// throw.
break;
}
emitExceptionCheck(*db, bbb);
break;
}
Expand Down
20 changes: 14 additions & 6 deletions RuntimeTests/hir_tests/dynamic_comparison_elimination_test.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ DynamicComparisonEliminationTest
---
DynamicComparisonElimination
---
IsBecomesComparison
EqualBecomesCompareBool
---
# HIR
fun test {
bb 0 {
v0 = LoadArg<0>
v1 = LoadArg<1>
v2 = Compare<Is> v0 v1
v2 = Compare<Equal> v0 v1
v3 = IsTruthy v2
CondBranch<1, 2> v3
}
Expand All @@ -29,7 +29,11 @@ fun test {
bb 0 {
v0:Object = LoadArg<0>
v1:Object = LoadArg<1>
v3:CBool = PrimitiveCompare<Equal> v0 v1
v3:CInt32 = CompareBool<Equal> v0 v1 {
FrameState {
NextInstrOffset 0
}
}
CondBranch<1, 2> v3
}

Expand All @@ -44,14 +48,14 @@ fun test {
}
}
---
IsNotBecomesComparison
NotEqualBecomesCompareBool
---
# HIR
fun test {
bb 0 {
v0 = LoadArg<0>
v1 = LoadArg<1>
v2 = Compare<IsNot> v0 v1
v2 = Compare<NotEqual> v0 v1
v3 = IsTruthy v2
CondBranch<1, 2> v3
}
Expand All @@ -71,7 +75,11 @@ fun test {
bb 0 {
v0:Object = LoadArg<0>
v1:Object = LoadArg<1>
v3:CBool = PrimitiveCompare<NotEqual> v0 v1
v3:CInt32 = CompareBool<NotEqual> v0 v1 {
FrameState {
NextInstrOffset 0
}
}
CondBranch<1, 2> v3
}

Expand Down
Loading

0 comments on commit e7d9af0

Please sign in to comment.