[PATCH] D100491: [X86] combineCMP - fold cmpEQ/NE(TRUNC(X),0) -> cmpEQ/NE(X,0)

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48835
+      APInt::getBitsSetFrom(TruncSrcVT.getSizeInBits(), VT.getSizeInBits());
+  if (TruncSrcVT.getSizeInBits() >= 32 &&
+      DAG.getTargetLoweringInfo().isTypeLegal(TruncSrcVT) &&
----------------
Do we create a i64 cmp if TruncSrcVT = i64? I think it's not efficient than the truncated CMP.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:48843
 
-  // Arithmetic op can only have one use.
-  if (!Op.hasOneUse())
-    return SDValue();
+  // After this the truncate and arithmetic op must have a single use.
+  if (!Op.hasOneUse() || !TruncSrc.hasOneUse())
----------------
It's a bit confusing. Maybe better either use
```
SDValue Trunc = Op;
Op = Op.getOperand(0);
...
if (!Trunc.hasOneUse() || !Op.hasOneUse())
```
or say the source and dest of the truncate must have a single use.


================
Comment at: llvm/test/CodeGen/X86/and-with-overflow.ll:66
 ; X86-NEXT:    andl $-17, %ecx
 ; X86-NEXT:    testw %cx, %cx
 ; X86-NEXT:    je .LBB2_2
----------------
Why is this test still kept?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100491