[PATCH] D152116: [DAGCombiner] Transform `(icmp eq/ne (and X,C0),(shift X,C1))` to use rotate or to getter constants.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:6192
+  if (ShiftOpc == ISD::SHL || ShiftOpc == ISD::SRL) {
+    assert(AndMask != std::nullopt &&
+           "Null andmask when querying about shift+and");
----------------
AndMask.has_value() ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:6210
+        return AndMask->getSignificantBits() > 32 ? ISD::SRL : ShiftOpc;
+      else
+        // We can only benefit if req at least 7-bit for the mask. We
----------------
(style) remove the else - the if case always returns:
```
// If the current setup has imm64 mask, then inverse will have
// at least imm32 mask (or be zext i32 -> i64).
if (VT == MVT::i64)
  return AndMask->getSignificantBits() > 32 ? ISD::SRL : ShiftOpc;

// We can only benefit if req at least 7-bit for the mask. We
// don't want to replace shl of 1,2,3 as they can be implemented
// with lea/add.
return ShiftOrRotateAmt.uge(7) ? ISD::SRL : ShiftOpc;
```


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:6230
+  // We only don't prefer rotate for scalar if we will get zext mask which is
+  // always from srl.
+  return ISD::SRL;
----------------
Please can you rephrase this? Its rather confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152116