[PATCH] D147266: [AArch64] Sink operands to allow for bitselect instructions

[Pranav Kant via Phabricator via cfe-commits]

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