[PATCH] D149027: [X86] Add peephole to convert `(Cmp Op32/Op64, Imm8)` -> `(Cmp Op16/Op8, Imm8)`

Sun Apr 23 17:17:44 PDT 2023

craig.topper added inline comments.

================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:5644
+    // during lowering.
+    if (OpVT.isScalarInteger() && OpVT.getScalarSizeInBits() > 8 &&
+        isa<ConstantSDNode>(N1) && !isNullConstant(N1) &&
----------------
goldstein.w.n wrote:
> craig.topper wrote:
> > OpVT.isScalarInteger() should always bee true for X86ISD::CMP I think. The isa<ConstantSDNode>(N1) would guarantee it too.
> > OpVT.isScalarInteger() should always bee true for X86ISD::CMP I think. The isa<ConstantSDNode>(N1) would guarantee it too.
> 
> There is `CMPrr`?
I may have written that poorly. I meant that if `isa<ConstantSDNode>(N1)` was true, it implied N1 had scalar integer type. Thus making the type isScalarInteger redundant even if  X86ISD::CMP didn't guarantee it.

================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:5665
+        SDValue TruncN1 = CurDAG->getConstant(
+            C.trunc(NewVT.getScalarSizeInBits()).getZExtValue(), dl, MVT::i32);
+        insertDAGNode(*CurDAG, SDValue(Node, 0), TruncN1);
----------------
goldstein.w.n wrote:
> craig.topper wrote:
> > Shouldn't the type for the constant be NewVT?
> Was doing that originally. Was running into:
> ```
> MachineOperand.h:370: llvm::Register llvm::MachineOperand::getReg() const: Assertion `isReg() && "This is not a register operand!"' failed.
> ```
> 
> Phoebe suggested this.
The types of the 2 arguments to X86ISD::CMP are supposed to be the same so there's some other issue going on.

================
Comment at: llvm/test/CodeGen/X86/combine-movmsk.ll:44
 ; SSE-NEXT:    movmskpd %xmm1, %eax
-; SSE-NEXT:    cmpl $3, %eax
+; SSE-NEXT:    cmpb $3, %al
 ; SSE-NEXT:    sete %al
----------------
Is this only a smaller encoding because it uses %al? For any other register I don't think it's smaller.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149027