[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:2158
+  case TargetOpcode::G_OR: {
+    /// Look for operations that take the lower `Width=Size-ShiftImm` bits of
+    /// `ShiftSrc` and insert them into the upper `Width` bits of `MaskSrc` via
----------------
paquette wrote:
> Why is this a doxygen comment?
Copy-pasta from an older version of the patch when it was on a helper function. I'll fix.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2190
+    I.eraseFromParent();
+    return constrainSelectedInstRegOperands(BFM, TII, TRI, RBI);
+  }
----------------
paquette wrote:
> I think `emitInstr` handles the constraining for you already:
> 
> ```
> MachineInstr *AArch64InstructionSelector::emitInstr(
> ...
>   constrainSelectedInstRegOperands(*MI, TII, TRI, RBI);
>   return &*MI;
> }
> ```
👍


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/select-bitfield-insert.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -o - -verify-machineinstrs -global-isel | FileCheck %s --check-prefixes=CHECK,GISEL
----------------
paquette wrote:
> Is it possible to use a MIR testcase instead?
IIUC, a MIR test case wouldn't show that we're doing as well as (and in some cases better than) SDAG, but I'm happy to convert it if you think that doesn't matter so much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103291