[AArch64] isDesirableToCommuteWithShift - fix UBFX masked shift comment. NFC.

[RKSimon]

Summary

Something that was confusing me while investigating the regressions in D129933 - the comment looks to be the wrong way around as isDesirableToCommuteWithShift is called with a shift op.

IIRC this is the pattern that isSeveralBitsExtractOpFromShr then uses.

[dmgreen]

I don't know this code very well. It looks like N = N->getOperand(0).getNode(); was added later than the original code.
The code seems to be checking for a AND with an operand that is a SRL. So in that regards matches the original comment. That pattern will itself be shifted by the original N, IIUC?
I expect the "((x >> C) & mask)" is meant to be the "extract of some number of bits from x". Those bits are then shifted into where they belong in the final value.

[RKSimon]

isDesirableToCommuteWithShift should only ever be called from a shift instruction - it wasn't planned to work the other way as well - I'd be happy to add asserts to the calls like we (mostly) have for shouldFoldConstantShiftPairToMask?

[dmgreen]

I think it is called with a shift - it is checking for _another_ shift further in the pattern. So the final pattern is (((x >> C) & mask) >> C2) or (((x >> C) & mask) << C2), but it's not checking much about the original C2 shift (and I'm not sure they are always constant or not). It looks like the original version of the function before https://reviews.llvm.org/D50667 was called with LHS, so the N = N->getOperand(0).getNode(); was added to compensate for passing the shift instead.

Adding asserts sounds useful. I also noticed that there was a comment on the base isDesirableToCommuteWithShift that says "though its operand,", that should say "through its operand,". And the formatting is off for the inner if here, if you are making changes. It sounds like updating the comment would be useful, but if you do I think it should have both shifts or make it clearer that N is not the original N. Maybe name it LHS instead?

[RKSimon]

Thanks - I missed that! I'll clean up the asserts in a sec and fix the though/through spelling

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

   return ScratchRegs;
 }
 
 bool
 AArch64TargetLowering::isDesirableToCommuteWithShift(const SDNode *N,
                                                      CombineLevel Level) const {
   N = N->getOperand(0).getNode();
   EVT VT = N->getValueType(0);
-    // If N is unsigned bit extraction: ((x >> C) & mask), then do not combine
+    // If N is unsigned bit extraction: ((x & mask) >> C), then do not combine
     // it with shift to let it be lowered to UBFX.
   if (N->getOpcode() == ISD::AND && (VT == MVT::i32 || VT == MVT::i64) &&
       isa<ConstantSDNode>(N->getOperand(1))) {
     uint64_t TruncMask = N->getConstantOperandVal(1);
     if (isMask_64(TruncMask) &&
       N->getOperand(0).getOpcode() == ISD::SRL &&
       isa<ConstantSDNode>(N->getOperand(0)->getOperand(1)))
       return false;