[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 marked 2 inline comments as done.
goldstein.w.n 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) &&
----------------
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`?


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:5646
+        isa<ConstantSDNode>(N1) && !isNullConstant(N1) &&
+        hasNoSignFlagUses(SDValue(Node, 0))) {
+      const APInt &C = cast<ConstantSDNode>(N1)->getAPIntValue();
----------------
craig.topper wrote:
> craig.topper wrote:
> > Do we need to check overflow flag uses too?
> Maybe carry flag too?
I think yes for overflow flag, but I don't think we need for carry flag.




================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149027