[DAG] Added m_AnyBinOp and m_c_AnyBinOp in SDPatternMatch.h #86435
Conversation
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The rationale behind going for a separate class here is: We would need to introduce PredFuncT in the template which would need changes in the template. This would necessitate changes in a large number of functions that use |
|
|
||
| template <typename LHS, typename RHS> | ||
| inline auto m_AnyBinOp(const LHS &L, const RHS &R) { | ||
| return AnyBinaryOp_match{[](const TargetLowering &TLI) { return true; }, L, R, |
There was a problem hiding this comment.
I think there is a misunderstanding here: the reason m_AnyBinOp is called "any" is because, as opposed to the existing m_BinOp who matches against a fixed opcode given by user, m_AnyBinOp will match any opcode that is TLI.isBinary/isCommutativeBinOp without any user-provided opcode to compare against with so you couldn't just return true here.
Furthermore, m_AnyBinOp(unsigned &Opc, LHS, RHS) is basically extracting the opcode that fulfills m_AnyBinOp(LHS, RHS). Therefore, I don't think PredFunc is necessary since both variants of m_AnyBinOp has to call TLI.isBinary/isCommutativeBinOp.
There was a problem hiding this comment.
Can you elaborate what you mean by:
Furthermore, m_AnyBinOp(unsigned &Opc, LHS, RHS) is basically extracting the opcode that fulfills m_AnyBinOp(LHS, RHS).
There was a problem hiding this comment.
Can you elaborate what you mean by:
Furthermore, m_AnyBinOp(unsigned &Opc, LHS, RHS) is basically extracting the opcode that fulfills m_AnyBinOp(LHS, RHS).
So m_AnyBinOp(LHS, RHS) would check if the SDNode's opcode fulfills TLI.isBinOP/isCommutativeBinOp; m_AnyBinOp(unsigned &Opc, LHS, RHS) is basically doing the same thing m_AnyBinOp(LHS, RHS) does, plus returning the opcode (that has been checked) via the Opc argument (as Opc has a type of unsigned reference, making it an output argument).
There was a problem hiding this comment.
I have updated the impl. Please let me know if that is what was required.
There was a problem hiding this comment.
Okay, so while I think that the m_AnyBinOp(unsigned &Opc, LHS, RHS) needs some work in the sense that I am still not returning the opcode, ie. giving Opcode the value of N->getOpcode(). But according to my testing, I don't think that isBinOp is being called at all. Any reason why?
There was a problem hiding this comment.
I don't think that isBinOp is being called at all
You could attach a debugger to see why isBinOp is skipped
| @@ -152,6 +152,24 @@ TEST_F(SelectionDAGPatternMatchTest, matchBinaryOp) { | |||
| sd_match(SFAdd, m_ChainedBinOp(ISD::STRICT_FADD, m_SpecificVT(Float32VT), | |||
| m_SpecificVT(Float32VT)))); | |||
|
|
|||
| EXPECT_TRUE(sd_match(And, m_AnyBinOp(ISD::AND, m_Value(), m_Value()))); | |||
There was a problem hiding this comment.
Have you ran this test? Because I'm a little surprised this test can be built and passed since the signature of AnyBinaryOp_match::match doesn't conform to what sd_match expects.
In case you don't know how to run a unit test, try ninja check-llvm-unit. Another way for people lacks patience like me is to explicitly build unit test via ninja UnitTests and run only the test in this file via
./unittests/CodeGen/CodeGenTests --gtest_filter='SelectionDAGPatternMatchTest.*'
(--gtest_filter comes from google test, which we use for unit testing)
|
Ah really sorry for the delayed response. My OS crashed. Apparently there were some dependency issues with my libstdc++ that happened recently due to which I was unable to run the tests locally. I pushed the changes to get insights from you. I did a clean build of my ubuntu22.04LTS and restored my system applications. Will push the changes today. |
| if ((Ctx.getTLI()->isBinOp(N->getOpcode()) || | ||
| (Commutable && Ctx.getTLI()->isCommutativeBinOp(N->getOpcode())))) { |
There was a problem hiding this comment.
| if ((Ctx.getTLI()->isBinOp(N->getOpcode()) || | |
| (Commutable && Ctx.getTLI()->isCommutativeBinOp(N->getOpcode())))) { | |
| if (Ctx.getTLI()->isBinOp(N->getOpcode()) || | |
| (Commutable && Ctx.getTLI()->isCommutativeBinOp(N->getOpcode()))) { |
Redundant paren
|
|
||
| template <typename LHS, typename RHS> | ||
| inline auto m_AnyBinOp(const LHS &L, const RHS &R) { | ||
| return AnyBinaryOp_match{[](const TargetLowering &TLI) { return true; }, L, R, |
There was a problem hiding this comment.
I don't think that isBinOp is being called at all
You could attach a debugger to see why isBinOp is skipped
Fixes: #84940