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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:5695
+      // TODO: Enable imm16 transform as well if -Os is set?
+      if (C.getSignificantBits() <= 8) {
+        EVT NewVT = OpVT;
----------------
craig.topper wrote:
> craig.topper wrote:
> > I think this can be `isInt<8>(C.getSExtValue())`
> The alive proof is checking the constant is zero extended from 8 bits,  right? But this check is a signed checked.
This is checking if `C` can be encoded as imm8.


================
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
----------------
craig.topper wrote:
> Is this only a smaller encoding because it uses %al? For any other register I don't think it's smaller.
> Is this only a smaller encoding because it uses %al? For any other register I don't think it's smaller.

You're right:
```
	cmpl	$-1, %eax
    cmpw    $-1, %ax
    cmpb    $-1, %al

	cmpl	$-1, %ecx
    cmpw    $-1, %cx
    cmpb    $-1, %cl    
```

Probably if anything we should do something to get rid of `imm16` encoding. But think its fair to abandon this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149027