[PATCH] D42088: [x86] shrink 'and' immediate values by setting the high bits (PR35907)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2461
+  KnownBits Known;
+  CurDAG->computeKnownBits(And0, Known);
+  if (Known.countMinLeadingZeros() < MaskLZ)
----------------
You could create a mask from the APInt::getHighBitSet call you need to do anyway and use the DAG.MaskedValueIsZero which wraps computeKnownBits?


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2466
+  // Don't bother changing the constant if it won't allow smaller encoding.
+  APInt NegVal = MaskVal | APInt::getHighBitsSet(VT.getSizeInBits(), MaskLZ);
+  unsigned MinWidth = NegVal.getMinSignedBits();
----------------
Should this check be before the more costly computeKnownBits?


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2471
+
+  SDLoc DL(And);
+  SDValue NewMask = CurDAG->getConstant(NegVal, DL, VT);
----------------
One of these days we should really decide between 'DL' and 'dl'...


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2473
+  SDValue NewMask = CurDAG->getConstant(NegVal, DL, VT);
+  SDValue NewAnd = CurDAG->getNode(ISD::AND, DL, VT, And0, NewMask);
+  ReplaceNode(And, NewAnd.getNode());
----------------
Just to make sure I understand. If the new mask is all ones, getNode constant folds the AND away leaving the constant disconnected. But isel will notice the constant is unused and not select it? Or maybe isel doesn't see the constant at all because it wasn't there when the selection process started?

Though if we can prove the upper bits are 0 and the result is all ones why didn't simplify demanded bits during DAG combine already remove the AND. I'm just trying to understand what happened on the tests in divide-by-constant.ll


https://reviews.llvm.org/D42088