This is an archive of the discontinued LLVM Phabricator instance.

[2/3][RISCV][POC] Model vxrm in LLVM intrinsics and machine instructions for RVV fixed-point instructions
ClosedPublic

Authored by eopXD on May 24 2023, 7:10 PM.

Details

Summary

Depends on D151395.

This is the 2nd patch of the patch-set. For the cover letter of the
patch-set, please checkout D151395. This patch originates from
D121376.

This commit models vxrm by adding an immediate operand into intrinsics
and machine instructions of RVV fixed-point instruction vaadd,
vaaddu, vasub, and vasubu. This commit only covers intrinsics of
the four instructions, the proceeding patches of the patch-set will do
the same to other RVV fixed-point instructions.

The current naiive approach is to have a write to vxrm inserted before
every fixed-point instruction. This is done by the new added pass
RISCVInsertReadWriteCSR. The reason to name the pass in a more general
term is because we will also model rounding mode for the RVV floating-
point instructions. The approach will be improved in the future,
implementing partial redundancy elimination algorithms to it.

The original LLVM intrinsics and machine instructions, take vaadd as
an example, does not model the rounding mode is not removed in this
patch. That is, int.riscv.vaadd.* co-exists with
int.riscv.vaadd.rm.* after this patch. The next patch will add C
intrinsics of vaadd with an additional operand that models the control
of the rounding mode, in this patch, int.riscv.vaadd.rm.* will
replace int.riscv.vaadd.*.

Authored-by: ShihPo Hung <shihpo.hung@sifive.com>
Co-Authored-by: eop Chen <eop.chen@sifive.com>

Diff Detail

Event Timeline

eopXD created this revision.May 24 2023, 7:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 7:10 PM
eopXD requested review of this revision.May 24 2023, 7:10 PM
eopXD updated this revision to Diff 525412.May 24 2023, 7:46 PM

Bump CI.

craig.topper added inline comments.May 24 2023, 8:19 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
113

Extra blank line

llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp
2

Incorrect file name and description

12

naive*

"insertion a write" -> "insertion of a write"?

84

Use operator* instead of optional::value. See many patches like 1792821c8308755593c114c3b7ae8ce33bdb08e9

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1938

80 characters

1939

This isn't indented far enought to line up with RetClass.

6455

I would kind of prefer you do this 4 instruction group together.

eopXD edited the summary of this revision. (Show Details)May 24 2023, 10:27 PM
eopXD updated this revision to Diff 525441.May 24 2023, 10:28 PM
eopXD marked 7 inline comments as done.

Address comments from Craig and resolve other test failures.

eopXD edited the summary of this revision. (Show Details)May 24 2023, 11:56 PM
eopXD edited the summary of this revision. (Show Details)May 25 2023, 12:21 AM
craig.topper added inline comments.May 25 2023, 9:35 PM
llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp
23

RISCV -> RISC-V

87

We need to add an implicit use of VXRM to the vector instruction. That's how we do it in the InsertVSETVLI pass. See

MI.addOperand(MachineOperand::CreateReg(RISCV::VL, /*isDef*/ false,      
                                        /*isImp*/ true));

from RISCVInsertVSETVLI::emitVSETVLIs

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
5787–5788

The Pseudos don't use VXRM until the VXRM write instruction is inserted.

5788

Line up the colons

llvm/test/CodeGen/RISCV/O0-pipeline.ll
44

RISCV -> RISC-V

llvm/test/CodeGen/RISCV/rvv/vxrm.mir
27–28

Should we delete the existing WriteVXRMImm instruction from this test?

eopXD updated this revision to Diff 525956.May 25 2023, 11:28 PM
eopXD marked 6 inline comments as done.

Address comments from Craig.

craig.topper added inline comments.May 26 2023, 12:00 PM
llvm/test/CodeGen/RISCV/rvv/vxrm.mir
17

I would expect there to be two $vxrm operands since you started with one and the pass added one.

28

Drop the implicit $vxrm here. It shouldn't exist until the pass runs.

The implicit $vl, implicit $vtype operands shouldn't exist until the InsertVSETVLI pass runs.

eopXD updated this revision to Diff 529173.Jun 6 2023, 11:06 PM
eopXD marked 2 inline comments as done.

Address comments from Craig.

llvm/test/CodeGen/RISCV/rvv/vxrm.mir
17

After removal of the starting $vxrm, I think it is correct here that one only have one here.

28

Removed implicit $vxrm, implicit $vl, implicit $vtype

craig.topper added inline comments.Jun 6 2023, 11:20 PM
llvm/test/CodeGen/RISCV/rvv/vxrm.mir
28

They still seem to be here?

eopXD updated this revision to Diff 529178.Jun 6 2023, 11:29 PM

Really delete the implit vl, vtype, and vxrm in test case.

eopXD marked an inline comment as done.Jun 6 2023, 11:29 PM
eopXD added inline comments.
llvm/test/CodeGen/RISCV/rvv/vxrm.mir
28

My bad, I seem to mix up the commits. Removed it.

eopXD marked an inline comment as done.Jun 6 2023, 11:30 PM
craig.topper added inline comments.Jun 8 2023, 8:57 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1132

Indentation

1936

Indentation

4526

Indentation

4664

Indentation

4720

Indentation

eopXD retitled this revision from [2/N][RISCV] Model vxrm in LLVM intrinsics and machine instructions for RVV fixed-point instructions to [2/3][RISCV][POC] Model vxrm in LLVM intrinsics and machine instructions for RVV fixed-point instructions.Jun 8 2023, 9:48 AM
eopXD updated this revision to Diff 529650.Jun 8 2023, 9:54 AM
eopXD marked 4 inline comments as done.

Fix indentation.

eopXD marked an inline comment as done.Jun 8 2023, 9:55 AM
This revision is now accepted and ready to land.Jun 8 2023, 11:15 AM
eopXD updated this revision to Diff 530403.Jun 11 2023, 11:46 PM

Rebase and resolve conflict.

eopXD updated this revision to Diff 530423.EditedJun 12 2023, 1:15 AM

Remove template VPseudoVAALU_VV_VX that is no longer in use.

eopXD updated this revision to Diff 531143.Jun 13 2023, 6:51 PM

Bump CI.

eopXD updated this revision to Diff 531645.Jun 15 2023, 1:46 AM

Rebase to latest main.

eopXD updated this revision to Diff 531646.Jun 15 2023, 1:48 AM

Remove side effect for Pseudo{VAADD/VAADDU/VASUB/VASUBU}.

eopXD updated this revision to Diff 531651.Jun 15 2023, 2:00 AM

Update code due to rebase.

eopXD updated this revision to Diff 531652.Jun 15 2023, 2:04 AM

Respect overall naming scheme for templates.

eopXD updated this revision to Diff 531653.Jun 15 2023, 2:06 AM

Fix indentation.

eopXD updated this revision to Diff 531674.Jun 15 2023, 3:13 AM

Update code due to rebase.

eopXD updated this revision to Diff 531688.Jun 15 2023, 3:57 AM

Remove let hasSideEffects = 0; under
VPseudoBinaryNoMaskRoundingMode, VPseudoBinaryNoMaskTURoundingMode, and VPseudoBinaryMaskPolicyRoundingMode.

eopXD updated this revision to Diff 532530.Jun 18 2023, 10:54 PM

Rebase to latest main.

eopXD updated this revision to Diff 532827.Jun 20 2023, 2:03 AM

Conform added templates to also have the isSEWAware parameter introduced by D152428.

FWIW, I/we (google) am seeing a failure internally on some Halide tests: LLVM ERROR: Cannot select: intrinsic %llvm.riscv.vaadd - I don't have a small repro to share, but might be worth reverting pre-emptively?

FWIW, I/we (google) am seeing a failure internally on some Halide tests: LLVM ERROR: Cannot select: intrinsic %llvm.riscv.vaadd - I don't have a small repro to share, but might be worth reverting pre-emptively?

I think this is because we changed the signature of the intrinsic, and Halide needs to be updated. Or we need to keep the old intrinsic names and their behavior for Halide. I think Halide ultimately does want the new behavior. Zalman had asked for it in the past.

FWIW, I/we (google) am seeing a failure internally on some Halide tests: LLVM ERROR: Cannot select: intrinsic %llvm.riscv.vaadd - I don't have a small repro to share, but might be worth reverting pre-emptively?

I think this is because we changed the signature of the intrinsic, and Halide needs to be updated. Or we need to keep the old intrinsic names and their behavior for Halide. I think Halide ultimately does want the new behavior. Zalman had asked for it in the past.

@zvookin Are you still working on Halide?