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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon 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) &&
----------------
pengfei wrote:
> Do we create a i64 cmp if TruncSrcVT = i64? I think it's not efficient than the truncated CMP.
It would, if we don't then we could still have cases where we fail to fold the compare to a i64 op's eflags result - but given most use cases of this patch is for promoted ops, then it will have just promoted from i8/i16 to i32 - so I'm happy to restrict it to i32. as well.


================
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())
----------------
pengfei wrote:
> 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.
Yes, that'd be better - will update it


================
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
----------------
pengfei wrote:
> Why is this test still kept?
Because i686 hasn't made use of the zeroext %0 tag, its become:

i32,ch = load<(load 2 from %fixed-stack.1, align 4), anyext from i16> t0, FrameIndex:i32<-1>, undef:i32

so known bits doesn't know the upper i16 should be zero.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100491