This is an archive of the discontinued LLVM Phabricator instance.

[X86] Transform `(cmp eq/ne trunc(X), C)` -> `(cmp eq/ne X, Zext(C))`
ClosedPublic

Authored by goldstein.w.n on Apr 17 2023, 5:33 PM.

Details

Summary

This previously existed for C == 0, but is mostly beneficial for any
C.

There is a slight codesize cost as we get more imm32 (as opposed to
imm8) constants in some cases. But the benefit is was get less imm16
constants (LCP stalls) and save instructions in some vec -> scalar
codegen.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Apr 17 2023, 5:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 5:34 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Apr 17 2023, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 5:34 PM

Should add non-zero tests?

llvm/lib/Target/X86/X86ISelLowering.cpp
54454

cast

dyn_cast -> cast

goldstein.w.n marked an inline comment as done.Apr 18 2023, 11:43 AM

Should add non-zero tests?

It felt like we saw enough changes throughout CodeGen/X86/ that it was sufficient. But if you prefer
I can add some new explicit tests for it.

RKSimon accepted this revision.Apr 19 2023, 2:42 AM

LGTM - we might want to consider a DAGISel peephole to take of cases where we lose signextended imm8 values.

llvm/lib/Target/X86/X86ISelLowering.cpp
54454

auto *

This revision is now accepted and ready to land.Apr 19 2023, 2:42 AM

LGTM - we might want to consider a DAGISel peephole to take of cases where we lose signextended imm8 values.

DAGISel? Which file is that? X86ISelDAGToDAG.cpp?

goldstein.w.n added a comment.EditedApr 19 2023, 6:58 PM

LGTM - we might want to consider a DAGISel peephole to take of cases where we lose signextended imm8 values.

DAGISel? Which file is that? X86ISelDAGToDAG.cpp?

Assume that.

I tried:

EVT OpVT = N0.getValueType();
if (OpVT.isScalarInteger() && OpVT.getScalarSizeInBits() > 8 &&
    !isNullConstant(N1)) {
  const APInt &C = cast<ConstantSDNode>(N1)->getAPIntValue();
  if (C.getSignificantBits() <= 8 &&
      CurDAG->MaskedValueIsZero(
          N0, APInt::getBitsSetFrom(OpVT.getScalarSizeInBits(), 8))) {
    SDValue TruncN0 = CurDAG->getZExtOrTrunc(N0, dl, MVT::i8);
    insertDAGNode(*CurDAG, SDValue(Node, 0), TruncN0);
    SDValue TruncN1 = getI8Imm(C.truncSSat(8).getZExtValue(), dl);
    insertDAGNode(*CurDAG, SDValue(Node, 0), TruncN1);
    SDValue NewCmp =
        CurDAG->getNode(X86ISD::CMP, dl, MVT::i32, TruncN0, TruncN1);
    ReplaceNode(Node, NewCmp.getNode());
    if (N1.getNode()->use_empty())
      CurDAG->RemoveDeadNode(N1.getNode());
    SelectCode(NewCmp.getNode());
    return;
  }
}

In X86ISD::CMP handling of select but fail in X86InstrInfo::analyzeCompare
with:

MachineOperand.h:370: llvm::Register llvm::MachineOperand::getReg() const: Assertion `isReg() && "This is not a register operand!"' failed.

See anything obviously wrong in home I'm setting this up?

This revision was landed with ongoing or failed builds.Apr 23 2023, 12:26 AM
This revision was automatically updated to reflect the committed changes.