[DAGCombiner] Make `(zext (sgt X, -1))` -> `(srl (not X), N-1)` work if typeof(zext)!=typeof(X)

[goldstein.w.n]

Summary

(srl (not X), N-1) is either 0/1, so any zext can be combined to
this.

To avoid the transformation when the result of the setcc is being
used directly, only allow mismatch if the zext type is larger than
i8.

[goldstein.w.n]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12804

Should this be behind a new TLI flag? Particularly not sure if the i8 case is generic or just x86.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2585

Don't bother with the if() - getZExtOrTrunc checks this anyway

12815

Don't bother with the if() - getZExtOrTrunc checks this anyway

[goldstein.w.n]

Remove conditions around zextortrunc

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

This is a somewhat x86 specific statement. i8 isn't a legal type on ARM/AArch64/RISC-V so setcc never has i8 type. It will be i1, i32, or i64 on those targets.

So what's special about i8 setcc on x86 that's different than the larger i32, i64 types on other targets?

[goldstein.w.n]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

This is a somewhat x86 specific statement. i8 isn't a legal type on ARM/AArch64/RISC-V so setcc never has i8 type. It will be i1, i32, or i64 on those targets.

So what's special about i8 setcc on x86 that's different than the larger i32, i64 types on other targets?

Its a hack for x86. simple code like return x > -1 will get zext the i1 to i8. In cases where we are just returning the flag, best not to do this transformation.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2585

For IsAdd don't we need getSExtOrTrunc? AFAICT we're after a 0/-1 value for Add and a 0/1 value for Sub?

[goldstein.w.n]

Fix add + sext case

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2585

For IsAdd don't we need getSExtOrTrunc? AFAICT we're after a 0/-1 value for Add and a 0/1 value for Sub?

Good catch, also added tests for this case.

[goldstein.w.n]

ping.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Ideally we still need a better way to do this that isn't so x86 specific

[goldstein.w.n]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Ideally we still need a better way to do this that isn't so x86 specific

Makes sense. I think the way to do this is check if the use of the zext is just a return.
I'm having trouble implementing that however. Do you know how to check if an SDNode is a ret?

[pengfei]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

N->isMachineOpcode() && getInstrInfo()->get(N->getMachineOpcode()).isReturn()

[goldstein.w.n]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Almost but llvm::TargetInstrInfo is still incomplete in DAGCombiner. I think will probably just make a TLI hook and disable this for other targets.

[pengfei]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Maybe DAG.getSubtarget().getInstrInfo()?
I don't think TargetInstrInfo is incomplete here. It's already used in SelectionDAGBuilder.

[goldstein.w.n]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Maybe DAG.getSubtarget().getInstrInfo()?
I don't think TargetInstrInfo is incomplete here. It's already used in SelectionDAGBuilder.

I tried that. This is what I used:

auto IsUsedForRet = [&](SDNode *Sn) {
  unsigned MaxUses = 50;
  for (auto Use : Sn->uses()) {
    if (Use->isMachineOpcode() && DAG.getSubtarget()
                                      .getInstrInfo()
                                      ->get(Use->getMachineOpcode())
                                      .isReturn())
      return true;

    // Limit incase use list is huge.
    if (MaxUses-- == 0)
      return true;
  }
  return false;
};

I'll play around a bit more before doing TLI version.

[pengfei]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Did you add its header #include "llvm/CodeGen/TargetInstrInfo.h"?

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

What makes a use by a return special? Why wouldn't the same apply to a live out of a basic block?

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12804

Wouldn't a return be represented by X86ISD::RET_GLUE at this point? It's not a machine opcode.

But also there would be a CopyToReg node as the user to connect the SDNode to a physical register like AL.

[pengfei]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12804

Yes, you are right, thanks @craig.topper

[goldstein.w.n]

Remove x86 hack

[goldstein.w.n]

Remove dead comment

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12801

Ideally we still need a better way to do this that isn't so x86 specific

Moved the hack to TLI.shouldAvoidTransformToShift so its only x86 now.