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

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

LGTM



================
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");
----------------
mingmingl wrote:
> 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.
Yep. Sounds good.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2602
+         "VT should be a simple scalar type");
+  Width = std::min(Width, (int)VT.getSizeInBits());
+
----------------
mingmingl wrote:
> 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.
There are quite a lot of words here for "we don't expect this to happen but it would be bad if it did" :)


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

https://reviews.llvm.org/D135852