[PATCH] D141464: [X86]: Match (xor TSize - 1, ctlz) to `bsr` instead of `lzcnt` + `xor`

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Just a couple of minor comments



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:52076
+                                 const X86Subtarget &Subtarget) {
+  assert(N->getOpcode() == ISD::XOR || N->getOpcode() == ISD::SUB);
+  if (Subtarget.hasFastLZCNT())
----------------
(style) assert message


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:52108
+  auto *C = dyn_cast<ConstantSDNode>(OpSizeTM1);
+  if (C == nullptr)
+    return SDValue();
----------------
if (!C)


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:52126
+    Op = DAG.getNode(ISD::TRUNCATE, DL, MVT::i8, Op);
+  }
+  return Op;
----------------
(style) remove braces


================
Comment at: llvm/test/CodeGen/X86/clz.ll:968
 ; the most significant bit, which is what 'bsr' does natively.
 ; FIXME: We should probably select BSR instead of LZCNT in these circumstances.
 define i32 @ctlz_bsr(i32 %n) {
----------------
pengfei wrote:
> The FIXME is solved :)
> But maybe better to leave a comment the `FASTLZCNT` is intended.
What target can manage 1c latency bsr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141464