[PATCH] D101062: [AArch64][SVE] Better utilisation of immediate forms for bitwise intrinsics

[Bradley Smith via Phabricator via llvm-commits]

bsmith added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4994
+  SDValue &Op = N;
+  while (Op.getOpcode() == AArch64ISD::REINTERPRET_CAST) {
+    Op = Op.getOperand(0);
----------------
sdesmalen wrote:
> I had expected there to be only a single REINTERPRET_CAST when it gets to the instruction selection phase, but I guess there are no combines that remove successive REINTERPRET_CASTs at the moment?
I'm not actually sure if there will always only be a single cast or not, but it feels like it's probably safer to just assume there isn't as it doesn't really add any complexity.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:354
 
+class SVE_1_Op_Imm_Log_Pred_Pat<ValueType vt, ValueType pt, SDPatternOperator op,
+                                ZPRRegOp zprty, ValueType it, ComplexPattern cpx, Instruction inst>
----------------
sdesmalen wrote:
> You're probably just following precedent, but I find the naming here a bit confusing since I'd think `_Pat` suggests the pattern takes a predicate. In this case, my preference would be to pass the `(SVEAllActive)` as operand/patfrag to the pattern class.
Perhaps changing this to be `SVE_1_Op_Pred_Imm_Log_Pat` would be better? (And changing the equivalent `Arith` version above). Passing in `(SVEAllActive)` in as a parameter just seems like it's adding parameters for the sake of it as nothing needs to pass this in differently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101062