[PATCH] D103105: [AArch64] Optimise bitreverse lowering in ISel

[Irina Dobrescu via Phabricator via llvm-commits]

Rin marked an inline comment as done.
Rin added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6875
+
+  assert(VT == MVT::v2i32 || VT == MVT::v4i32 || VT == MVT::v1i64 ||
+         VT == MVT::v2i64);
----------------
dmgreen wrote:
> If we have the unreachable below, we probably don't need the assert as well.
I'll get rid of it


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6886
+  case MVT::v2i32: {
+    REVB = DAG.getNode(AArch64ISD::REV32, DL, MVT::v8i8, Op.getOperand(0));
+
----------------
dmgreen wrote:
> As the code in each of these case blocks are very similar, is it worth having the switch just set some variables (BitReverseType and RevOpcode), and having the actual code shared below the switch?
Okay, I'll take care of that then.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:6872
+    return LowerToPredicatedOp(Op, DAG, AArch64ISD::BITREVERSE_MERGE_PASSTHRU,
+                               true);
+  }
----------------
dmgreen wrote:
> Rin wrote:
> > SjoerdMeijer wrote:
> > > Nit: `/*OverrideNEON=*/true` ?
> > > 
> > > And we don't need the curly brackets for this if.
> > Should I get rid of the 
> > ```
> > /*OverrideNEON=*/true
> > ```
> > 
> > above in LowerCTTZ as well?
> I think he meant to add /*OverrideNEON=*/, as it makes the code easier to read.
Oh, I misunderstood, I'll add it back in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103105