This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Optimize SELECT_CC when the true value of select is Constant
ClosedPublic

Authored by liaolucy on Jul 14 2022, 4:57 AM.

Details

Summary

(select (setcc lhs, rhs, CC), constant, falsev) -> (select (setcc lhs, rhs, InverseCC), falsev, constant)

This patch removes unnecessary copies

Diff Detail

Event Timeline

liaolucy created this revision.Jul 14 2022, 4:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
liaolucy requested review of this revision.Jul 14 2022, 4:57 AM

Without mention SELECT_CC, this title sounds very confusing.

liaolucy retitled this revision from [RISCV] Optimize ISD::SETEQ to ISD::SETNE when the true value is zero to [RISCV] Optimize SELECT_CC when condition is eq and the true value of select is zero.Jul 14 2022, 6:14 PM
liaolucy edited the summary of this revision. (Show Details)
liaolucy removed a reviewer: sjarus.
craig.topper added inline comments.Jul 14 2022, 6:20 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4299

This transform is equally valid to turn SETNE into SETEQ right?

liaolucy updated this revision to Diff 444906.Jul 15 2022, 1:00 AM

address craig.topper's comments and thanks

liaolucy edited the summary of this revision. (Show Details)Jul 15 2022, 1:02 AM
craig.topper added inline comments.Jul 16 2022, 10:50 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4299

This is valid for any condition code isn't it? We just need to invert the condition code.

liaolucy updated this revision to Diff 445320.Jul 17 2022, 8:59 AM
  1. address craig.topper's comments, remove condition code check.
  1. Also remove ConstantSDNode->isZero().
liaolucy retitled this revision from [RISCV] Optimize SELECT_CC when condition is eq and the true value of select is zero to [RISCV] Optimize SELECT_CC when the true value of select is Constant.Jul 17 2022, 9:01 AM
liaolucy edited the summary of this revision. (Show Details)
craig.topper added inline comments.Jul 17 2022, 5:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4210

There's already ISD::getSetCCInverse in ISDOpcodes.h

liaolucy updated this revision to Diff 445371.Jul 17 2022, 7:15 PM

address craig.topper's comments , thanks

frasercrmck added inline comments.Jul 18 2022, 2:59 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4302

InverseCC might be more canonical here?

4305

Could we instead just std::swap(TrueV, FalseV) and let it fall through? I think that's clearer to read.

asb added a comment.Jul 18 2022, 3:10 AM

Did you investigate any cases where this results in slightly worse codegen? one example would be 20100416-1.c from the GCC torture suite (-march=rv64imafdc -mabi=lp64d -O3). Overall the change seems to be positive, but there do seem to be a few cases like this where enabling this transformation results in an increase in instructions.

It needn't necessarily block this patch, as it does seem like a sensible transformation to make - but it would be useful to understand why these regressions occur, and capture some representative test cases.

liaolucy updated this revision to Diff 445672.EditedJul 18 2022, 6:56 PM
  1. Address @frasercrmck's comments
  2. Address @asb's comments. Fix negative optimization of 20100416-1.c from the GCC torture suite. No optimization is required when TrueV and FalseV are both constants.

selectcc-to-shiftand.ll has an example already covering this type.

define i32 @pos_sel_constants(i32 signext %a) {
; CHECK-LABEL: pos_sel_constants:
; CHECK:       # %bb.0:
; CHECK-NEXT:    mv a1, a0
; CHECK-NEXT:    li a0, 5
; CHECK-NEXT:    bgez a1, .LBB4_2
; CHECK-NEXT:  # %bb.1:
; CHECK-NEXT:    li a0, 0
; CHECK-NEXT:  .LBB4_2:
; CHECK-NEXT:    ret
  %tmp.1 = icmp sgt i32 %a, -1
  %retval = select i1 %tmp.1, i32 5, i32 0
  ret i32 %retval
}
liaolucy edited the summary of this revision. (Show Details)Jul 18 2022, 6:58 PM
liaolucy edited reviewers, added: frasercrmck; removed: sjarus.
jrtc27 added inline comments.Jul 18 2022, 7:10 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4305

This still isn't falling through. Ops and the return are identical to outside the if, you can just reuse them.

asb added a comment.Jul 19 2022, 1:04 AM
  1. Address @frasercrmck's comments
  2. Address @asb's comments. Fix negative optimization of 20100416-1.c from the GCC torture suite. No optimization is required when TrueV and FalseV are both constants.

selectcc-to-shiftand.ll has an example already covering this type.

Thanks, that fixes that regression. I also see increased instruction counts for other tests including: pr20100-1, pr25125, pr37102, pr68506, pr83383, pr89634 (O3, rv64imafdc, lp64d). The overall impact across the suite seems positive, but it's likely worth taking a look.

Thanks, that fixes that regression. I also see increased instruction counts for other tests including: pr20100-1, pr25125, pr37102, pr68506, pr83383, pr89634 (O3, rv64imafdc, lp64d). The overall impact across the suite seems positive, but it's likely worth taking a look.

I used a stupid way to count valid static instructions. Dump .s file and remove comments, label and other useless information. If there is a better way I am willing to try it.

pr20100-1: The number of instructions is the same, both are 70.

pr37102, pr68506, pr89634: This patch has 1 more j instructions.
pr83383: This patch has 2 more j instructions.

Eg, from Pr83383.c: 
original:
       li      a0, 0
       beqz    a4, .LBB0_2

This patch:
       bnez    a3, .LBB0_2
newbb: li      a0, 0 
       j       .LBB0_3      -------------------------------- +1

Pr25125.c: +3 instructions.

original:
f:                                      # @f
# %bb.0:                                # %entry
        li      a1, 0
        bgtz    a0, .LBB0_2
# %bb.1:                                # %entry
        lui     a1, 8
        xor     a1, a1, a0
.LBB0_2:                                # %entry
        slli    a0, a1, 48
        srli    a0, a0, 48
        ret


This patch:
f:                                      # @f
# %bb.0:                                # %entry
        blez    a0, .LBB0_2
# %bb.1:                                # %entry
        li      a0, 0
        slli    a0, a0, 48  --------------------copy--------- +1
        srli    a0, a0, 48  --------------------copy--------- +2
        ret                 --------------------copy--------- +3
.LBB0_2:
        lui     a1, 8
        xor     a0, a0, a1
        slli    a0, a0, 48
        srli    a0, a0, 48
        ret
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4305

I'll update it back later.
SDValue Ops[] = {LHS, RHS, TargetCC, FalseV, TrueV,};

liaolucy updated this revision to Diff 447581.Jul 25 2022, 11:52 PM

Address jrtc27's comment

craig.topper added inline comments.Jul 26 2022, 9:20 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4305

This is still not the addressing the comment. This was the request

if (isa<ConstantSDNode>(TrueV) && !isa<ConstantSDNode>(FalseV)) {
  std::swap(TrueV, FalseV)
  TargetCC =
      DAG.getCondCode(ISD::getSetCCInverse(CCVal, LHS.getValueType()));
}

SDValue Ops[] = {LHS, RHS, TargetCC, TrueV, FalseV};
return DAG.getNode(RISCVISD::SELECT_CC, DL, Op.getValueType(), Ops);
liaolucy updated this revision to Diff 447920.Jul 26 2022, 8:49 PM

Update. I am sorry that I didn't understand well. Thanks,craig.topper.

I lost track of this patch for a while, but I just stumbled onto a case that would benefit. Can you rebase this?

This revision is now accepted and ready to land.Oct 17 2022, 9:15 AM
This revision was landed with ongoing or failed builds.Oct 17 2022, 6:24 PM
This revision was automatically updated to reflect the committed changes.