[PATCH] D135852: [AArch64] Enhance bit-field-positioning op matcher to see through "any_extend" for pattern "and(any_extend(shl(val, N)), shifted-mask)"

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2573
+    // For pattern "and(shl(val, N), shifted-mask)", 'Src' is set to 'val'.
+    Src = AndOp0.getOperand(0);
+  } else if (VT == MVT::i64 && AndOp0.getOpcode() == ISD::ANY_EXTEND &&
----------------
dmgreen wrote:
> Can we create a new variable for the temporary SDValue?
Renamed this to 'ShlOp0'


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2577
+                                   ShlImm)) {
+    // For pattern "and(any_extend(shl(val, N)), shifted-mask)"
+
----------------
dmgreen wrote:
> Add an assert that the type of ShlVal is i32?
Add the type assertion and comment for why ShlVal type is i32 (based on my understanding).


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2600
+  // clamp Width according to source bit width.
+  assert(VT.isSimple() && VT.isScalarInteger() &&
+         "VT should be a simple scalar type");
----------------
dmgreen wrote:
> I think this will always be a MVT::i64 or MVT::i32?
Yes, I agree this should always be i64 or i32 after reading the comment and think about it.

p.s. if I understand correctly, this is because 
1) this function gets called during selection (i.e., after legalization) and caller verifies i64 or i32 above; &&
2) AArch64 general purpose registers are for 32 or 64 bit integers, and SimpleVT legalization (from [[ https://github.com/llvm/llvm-project/blob/4e5568d92d2cebdb06b4255ecf5e161acff55a2b/llvm/lib/CodeGen/TargetLoweringBase.cpp#L958 | target-lowering-base) class]] is based on register info.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2602
+         "VT should be a simple scalar type");
+  Width = std::min(Width, (int)VT.getSizeInBits());
+
----------------
dmgreen wrote:
> Do we have any tests where this applies?
This is a good point. I think the answer is no (or at least very rare if any) -> changed implementation to bail out if `Width >= VT.getSizeInBits()` and added comment. 

Longer story is that, if `AndImm` (as a superset of shifted-mask 'NonZeroBits`) from `and(op, AndImm)` requires more than 32 bits after proper dag combination, the `op` shouldn't be lowered to `any_extend (smaller-type op)`
- the higher bits are undefined after `any_extend`, so previous combination should at least realize the higher 32 bits and results  don't really matter.


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

https://reviews.llvm.org/D135852