pranavk planned changes to this revision.
pranavk marked 2 inline comments as done.
pranavk added inline comments.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14343
+ // passed to this function. Starting pattern matching with any other
+ // instruction (such as Or) would lead to malformed IR
+
----------------
dmgreen wrote:
> That sounds like it might be a bug that happens if it tries to sink too many
> operands? From what I remember the order they are put into Ops might matter.
> And if it is sinking to the Or it might need to add both the And as well as
> the Not.
When I tried to sink Or, I didn't add both Ands in the Vector. So it was
sinking it just before Or even though it was used by And one instruction before
the location where it sunk it. So, I don't think it's a bug. I just forgot to
add Ands to the vector. I tried adding Ands to the vector and it works. So I
have changed my implementation to switch-case under Instruction::Or as I think
that makes it easier to read this code.
================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14364
+ for (unsigned AndOpIndex = 0; AndOpIndex < 2; ++AndOpIndex) {
+ if (const Instruction *XI =
+ dyn_cast<Instruction>(Ands[AndIndex][AndOpIndex]);
----------------
dmgreen wrote:
> Can this be simplified with a m_Not matcher? In general instructions will be
> canonicalized so that constants are on the RHS.
>
> If we are sinking the Not, I feel this should want to test that the pattern
> makes up a bsl, and if it does then sink the operand of I. i.e. something
> like checking that OI is
> `m_c_Or(m_c_And(m_Value(A),m_Value(B)),m_specific(I))`, and then checking
> that `I` is `m_c_And(m_Not(m_Specific(A)),m_Value(D))` or the other way
> around.
Absolutely. I didn't look close enough it seems in PatternMatcher.h file. After
discovering whole bunch of pattern matchers, I have shortened/simplified this
implementation using m_not, etc. Please look at the latest patch.
I have additionally added more guards/checks to prevent this from happening
when one of the And, or its operands are not available in same basic block.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147266/new/
https://reviews.llvm.org/D147266
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
