This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by goldstein.w.n on Feb 28 2023, 1:20 PM.

Details

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.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Feb 28 2023, 1:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 1:20 PM
goldstein.w.n requested review of this revision.Feb 28 2023, 1:20 PM
goldstein.w.n added inline comments.
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 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2585

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

12817

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

goldstein.w.n marked 2 inline comments as done.Feb 28 2023, 2:11 PM

Remove conditions around zextortrunc

craig.topper added inline comments.Mar 1 2023, 11:30 AM
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 added inline comments.Mar 1 2023, 11:40 AM
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 added inline comments.Mar 2 2023, 4:04 AM
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 marked an inline comment as done.

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.

RKSimon added inline comments.Apr 11 2023, 1:50 AM
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 added inline comments.Apr 11 2023, 1:04 PM
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 added inline comments.Apr 11 2023, 7:53 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12801

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

goldstein.w.n added inline comments.Apr 11 2023, 10:55 PM
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.

goldstein.w.n marked an inline comment as not done.Apr 11 2023, 10:56 PM
pengfei added inline comments.Apr 11 2023, 11:09 PM
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 added inline comments.Apr 12 2023, 12:25 AM
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 added inline comments.Apr 12 2023, 12:46 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12801

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

craig.topper added inline comments.Apr 12 2023, 12:47 AM
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 added inline comments.Apr 12 2023, 12:52 AM
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 added inline comments.Apr 12 2023, 1:04 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12804

Yes, you are right, thanks @craig.topper

Remove x86 hack

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.