This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] The immediate version of sgt/ugt lowering to slti/sltiu + xori
ClosedPublic

Authored by Miss_Grape on Mar 18 2022, 5:54 PM.

Diff Detail

Event Timeline

Miss_Grape created this revision.Mar 18 2022, 5:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2022, 5:54 PM
Miss_Grape requested review of this revision.Mar 18 2022, 5:54 PM

There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?

There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?

Agreed, I'd at least expect the same for setugt.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
354

Can we use [-2049, 2046] to describe the range like the comments on AddiPair? That's a more common way to represent a range. The ~ character in there is weird. Please update the comment on simm12_plus1 as well.

There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?

Agreed, I'd at least expect the same for setugt.

Though I guess that requires a special case for the discontinuity around 0.

Miss_Grape retitled this revision from [RISCV] The immediate version of sgt lowering to slti + xori to [RISCV] The immediate version of sgt/ugt lowering to slti/sltiu + xori.
Miss_Grape edited the summary of this revision. (Show Details)
craig.topper added inline comments.Mar 21 2022, 12:47 AM
llvm/test/CodeGen/RISCV/arith-with-overflow.ll
32 ↗(On Diff #416831)

This looks like a regression

There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?

I have add sgt/ugt ,and there are support RV32/RV64

There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?

Agreed, I'd at least expect the same for setugt.

done

benshi001 added inline comments.Mar 21 2022, 1:01 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

The zero value should be excluded, which is not needed to do li, and case just
be used with the register X0.

Miss_Grape added inline comments.Mar 21 2022, 1:04 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
354

done

llvm/test/CodeGen/RISCV/arith-with-overflow.ll
32 ↗(On Diff #416831)

sgtz a3 a1 ------> slt a3, X0, a1
slt a3, X0, a1---->slti a1, a1, 1 + xori a1, a1, 1
I think the result of the above assembly conversion is correct, which can reduce the registers use.

benshi001 added inline comments.Mar 21 2022, 1:09 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

I think

  1. The form (-2049 <= Imm && Imm < 0) || (0 < Imm && Imm <= 2046) is more clear.
  1. Do a further check that the imm node has only one use.
Miss_Grape added inline comments.Mar 21 2022, 4:59 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

I don't think we need to specifically consider the case of Imm == 0, riscv-gcc did not exclude it alone

craig.topper added inline comments.Mar 21 2022, 9:08 AM
llvm/test/CodeGen/RISCV/arith-with-overflow.ll
32 ↗(On Diff #416831)

If a1 was free to use as a destination register then it could have been sgtz a1, a1. This patch just moved the slti+xor below the the sub. We could do the same thing with the sgtz but it wouldn't change the number of registers needed.

khchen added a subscriber: khchen.Mar 21 2022, 8:56 PM
khchen added inline comments.
llvm/test/CodeGen/RISCV/float-fcmp-strict.ll
72

It seems no any benefits in this case, is it expected?

Miss_Grape added inline comments.Mar 22 2022, 1:14 AM
llvm/test/CodeGen/RISCV/float-fcmp-strict.ll
72

sgtz ---->slt rd, X0, rs2
slt rd, 0 rs2 ---> slti + xori
which can reduce the usage of one register

Miss_Grape added inline comments.Mar 22 2022, 1:30 AM
llvm/test/CodeGen/RISCV/arith-with-overflow.ll
32 ↗(On Diff #416831)

Do you want me to exclude scenarios where Imm == 0 ?

immediate version of sgt/ugt lowering to slti/sltiu + xori, and exclude imm == 0

Miss_Grape added inline comments.Mar 22 2022, 2:47 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

done

llvm/test/CodeGen/RISCV/float-fcmp-strict.ll
72

If this is the case(sgtz x1,x1), there is really no benefit,so I have excluded the case where the immediate value is 0

Add test for sgt/ugt, which Imm == 0

benshi001 added inline comments.Mar 22 2022, 3:25 AM
llvm/test/CodeGen/RISCV/fpclamptosat_vec.ll
1484 ↗(On Diff #417228)

This case becames poor, due to a0 (containing -1) has mutlple uses. So I think your need an extra check, that the immediate (simm12_sub1_exc0) has only one use.

Increase the limit that immediate can only be used once

benshi001 added inline comments.Mar 25 2022, 3:11 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1222

For this setugt, it would be better to make a comment, the coresspondant positve value range of [-2049, -1], which is different on rv32 and rv64.

llvm/test/CodeGen/RISCV/i32-icmp.ll
228

Though the code is correct, it would be better to use positive number, which is more clear.

benshi001 added inline comments.Mar 25 2022, 3:13 AM
llvm/test/CodeGen/RISCV/i32-icmp.ll
179

We need a more test case of -2050, which is the lower bound of simm12_sub1_exc0

craig.topper added inline comments.Mar 25 2022, 3:31 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
360

There's an extra space before the third use of Imm

craig.topper added inline comments.Mar 25 2022, 3:36 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

Use simm12_sub1_nonzero that's consistent with simm5_plus1_nonzero from RISCVInstrInfoV.td

llvm/test/CodeGen/RISCV/i32-icmp.ll
1

Is there a similar test file for rv64?

Correction review comments

Miss_Grape added inline comments.Mar 26 2022, 9:28 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

done

360

done

llvm/test/CodeGen/RISCV/i32-icmp.ll
1

I have add i64-icmp.ll test, and just test sgt/ugt. I will resubmit a new patch for anthor icmp tests

179

done

craig.topper added inline comments.Mar 27 2022, 1:29 PM
llvm/test/CodeGen/RISCV/i64-icmp.ll
5 ↗(On Diff #418433)

Shouldn't this use i64 instead of i32 to match the file name?

Updata test

Miss_Grape added inline comments.
llvm/test/CodeGen/RISCV/i64-icmp.ll
5 ↗(On Diff #418433)

done

craig.topper accepted this revision.Mar 28 2022, 10:25 PM

LGTM with that change.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

Use minus1 instead of sub1 to match better with the plus1 PatFrag abobe.

This revision is now accepted and ready to land.Mar 28 2022, 10:25 PM
This revision was landed with ongoing or failed builds.Mar 28 2022, 11:47 PM
This revision was automatically updated to reflect the committed changes.
benshi001 added inline comments.Mar 29 2022, 2:32 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
355

sorry, I did not notice your comments when commiting, and I will change that to minus1 in my next commit.