[PATCH] D103291: [AArch64][GISel] and+or+shl => bfi

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2164
+
+    if (Ty != LLT::scalar(32) && Ty != LLT::scalar(64))
+      return false;
----------------
aemerson wrote:
> Aesthetic opinion but no big deal: it's nicer to use:
> ```
> if (!Ty.isScalar())
>   return false;
> if (Size != 32 && Size != 64)
>   return false;
> ```
> and bring the `unsigned Size = ...` up to here.
ooh, I like that better too.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2171
+    int64_t MaskImm;
+    if (!mi_match(Dst, MRI,
+                  m_GOr(m_OneNonDBGUse(m_GShl(m_Reg(ShiftSrc), m_ICst(ShiftImm))),
----------------
aemerson wrote:
> Can we avoid re-matching on Dst here? We already know that MI == a G_OR, so we can just match the individual operands.
I considered that, but we're getting one more thing from the `m_GOR()` here: the fact that it matches both orders of its operands via that commutative check in `BinaryOp_match::match()`. Is there a clean way to write that inline, or should I add another matcher for it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103291/new/

https://reviews.llvm.org/D103291