[PATCH] D114354: [DAG] SimplifyDemandedBits - simplify rotl/rotr to shl/srl

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:1749-1755
+      // See if we don't demand either half of the rotated bits.
+      if ((!TLO.LegalOperations() || isOperationLegal(ISD::SHL, VT)) &&
+          DemandedBits.countTrailingZeros() >= (IsROTL ? RevAmt : Amt)) {
+        if (!IsROTL)
+          Op1 = TLO.DAG.getConstant(RevAmt, dl, Op1.getValueType());
+        return TLO.CombineTo(Op, TLO.DAG.getNode(ISD::SHL, dl, VT, Op0, Op1));
+      }
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > Are you sure this is right?
> > https://alive2.llvm.org/ce/z/EJDzlC
> Perhaps this is what you had in mind:
> * https://alive2.llvm.org/ce/z/iroxki (rol -> shl by amt iff demanded bits has at least as many trailing zeros as the shift amount)
> * https://alive2.llvm.org/ce/z/4ez_U- (ror -> shl by revamt iff demanded bits has at least as many trailing zeros as the reverse shift amount)
> * https://alive2.llvm.org/ce/z/cD7dR- (ror -> lshr by amt iff demanded bits has at least as many leading zeros as the shift amount)
> * https://alive2.llvm.org/ce/z/_XGHtQ (rol -> lshr by revamt iff demanded bits has at least as many leading zeros as the reverse shift amount)
Thanks @lebedev.ri !


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:17052
   EVT VT = N->getValueType(0);
-  if (N->getOpcode() == ISD::SRL && VT == MVT::i32 && ST->hasV6Ops()) {
-    // Canonicalize (srl (bswap x), 16) to (rotr (bswap x), 16) if the high
----------------
dmgreen wrote:
> craig.topper wrote:
> > Can this be done in isel pattern? There's an existing top16Zero PatFrag that calls MaskedValueIsZero in ARMInstrThumb2.td
> That does sound good if it will work. It would be a good way to keep the same pattern working, and I would say a tablegen pattern is preferable to a new node type.
> 
> I noticed that llvm.bswap.i16 would no longer generate a rev16, which would be a shame to see. The same thing didn't seem to happen on AArch64 though, it was still fine. I'm not entirely sure what the difference was.
Sorry - I missed these comments before doing my own version, which handles the bswap_upperzero specific case. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114354