This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][POC] Model frm control for vfadd
ClosedPublic

Authored by eopXD on Jun 15 2023, 12:53 AM.

Details

Summary

Depends on D152879.

Specification PR: riscv-non-isa/rvv-intrinsic-doc#226

This patch adds variant of vfadd that models the rounding mode control.
The added variant has suffix _rm appended to differentiate from the
existing ones that does not alternate frm and uses whatever is inside.

The value 7 is used to indicate no rounding mode change. Reusing the
semantic from the rounding mode encoding for scalar floating-point
instructions.

Additional data member HasFRMRoundModeOp is added so we can append
_rm suffix for the fadd variants that models rounding mode control.

Additional data member IsRVVFixedPoint is added so we can define
pseudo instructions with rounding mode operand and distinguish the
instructions between fixed-point and floating-point.

Diff Detail

Event Timeline

eopXD created this revision.Jun 15 2023, 12:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2023, 12:53 AM
eopXD requested review of this revision.Jun 15 2023, 12:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 15 2023, 12:53 AM
eopXD edited the summary of this revision. (Show Details)Jun 15 2023, 4:56 AM
eopXD updated this revision to Diff 531701.Jun 15 2023, 4:56 AM
eopXD edited the summary of this revision. (Show Details)

Resolve test case failures.

eopXD updated this revision to Diff 531722.Jun 15 2023, 6:17 AM

Update code based on edit of parent revision.

craig.topper added inline comments.Jun 15 2023, 9:25 AM
llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp
95

Don't we need to restore the original FRM value after the vector instruction?

eopXD added inline comments.Jun 15 2023, 9:45 AM
llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp
95

Yes, will fix this in the next diff.

eopXD updated this revision to Diff 532536.Jun 18 2023, 11:16 PM

Update code:

  • Change value to indicate no rounding mode change from 99 to 7.
  • Add code under RISCVDAGToDAGISel::performCombineVMergeAndVOps to deal with the extra rounding mode operand
  • This patch was originally depending upon D152889, the patch is dropped and now this patch depends on D152879.
eopXD edited the summary of this revision. (Show Details)Jun 18 2023, 11:17 PM
eopXD updated this revision to Diff 532796.Jun 19 2023, 11:39 PM
eopXD marked an inline comment as done.

Save and restore FRM in RISCVInsertReadWriteCSR.

eopXD updated this revision to Diff 532797.Jun 19 2023, 11:51 PM

Remove unnecessary include-s.

eopXD updated this revision to Diff 532800.Jun 20 2023, 12:03 AM

Fix ManualCodegen for vfadd.

eopXD updated this revision to Diff 532836.Jun 20 2023, 2:30 AM

Rebase: Add sew parameter that other templates have added too.

eopXD updated this revision to Diff 532839.Jun 20 2023, 2:41 AM

Add SemaChecking and corresponding test case.

eopXD updated this revision to Diff 532996.Jun 20 2023, 11:19 AM

Rebase to latest main.

eopXD added a comment.Jun 26 2023, 2:25 AM

Gentle ping.

craig.topper added inline comments.Jun 26 2023, 4:06 PM
llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp
104

We still need to add the RISCV::FRM operand for FRMImm == 7. But we should probably do it in RISCVTargetLowering::AdjustInstrPostInstrSelection

llvm/lib/Target/RISCV/RISCVInstrFormats.td
226

Something like UsesVXRM is probably better? As we established before, not everything the vector spec calls fixed point uses VXRM. Or make HasRoundOp a 2-bit field that encodings what type of rounding mode?

eopXD updated this revision to Diff 535425.Jun 28 2023, 8:29 AM
eopXD marked 2 inline comments as done.

Address comment from Craig.

craig.topper added inline comments.Jun 28 2023, 8:58 AM
llvm/lib/Target/RISCV/RISCVInsertReadWriteCSR.cpp
94

Use RISCVFPRndMode::DYN?

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
114

Inert -> Insert

115

Can we use FRM_DYN here? It's defined in RISCVInstrInfoF.td

214

Insert

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
633

Insert

846

Insert

eopXD updated this revision to Diff 535438.Jun 28 2023, 9:08 AM
eopXD marked 6 inline comments as done.

Address more comments from Craig.

craig.topper added inline comments.Jun 28 2023, 9:18 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
21

We can't include a CodeGen header in MCTargetDesc

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

Don't write 7 here

eopXD updated this revision to Diff 535446.Jun 28 2023, 9:36 AM
eopXD marked 2 inline comments as done.

Address more comments from Craig.

This revision is now accepted and ready to land.Jun 28 2023, 9:42 AM
kito-cheng accepted this revision.Jun 30 2023, 12:48 AM

LGTM, Give my blessing, thanks for moving this forward :)

eopXD updated this revision to Diff 536688.Jul 3 2023, 2:04 AM

Update suffix from

vfadd_vv_i32m1_{policy_suffix, if any}_rm to vfadd_vv_i32m1_rm_{policy_suffix, if any}

eopXD updated this revision to Diff 536699.Jul 3 2023, 2:53 AM

Fix bug in ManualCodeGen of vfadd.

eopXD updated this revision to Diff 536700.Jul 3 2023, 3:02 AM

Undo the previous diff. There was no bug in the code.

eopXD updated this revision to Diff 536964.Jul 3 2023, 11:43 PM

Resolve test case failure in llvm/test/CodeGen/RISCV/rvv/combine-vmv.ll

eopXD updated this revision to Diff 538748.Jul 10 2023, 11:23 AM

Fix bug, should be checking MI.readsRegister(RISCV::FRM) under AdjustInstrPostInstrSelection.

eopXD updated this revision to Diff 539222.Jul 11 2023, 11:49 AM

Under RISCVInsertReadWriteCSR, add implicit depdendency to MI if rounding mode is FRM_DYN.

eopXD updated this revision to Diff 539413.Jul 12 2023, 12:24 AM

Change:

  • Rebase to latest main
  • let hasPostISelHook = 1 for PseudoVFADD to trigger code under AdjustInstrPostInstrSelection.
craig.topper added inline comments.Jul 12 2023, 8:55 AM
clang/lib/Sema/SemaChecking.cpp
4816

not related to this patch, but the tama here doesn't match the intrinsic naming.

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

Keep the let on one line

eopXD added inline comments.Jul 12 2023, 10:39 AM
clang/lib/Sema/SemaChecking.cpp
4816

Just created https://reviews.llvm.org/D155102, will rebase the patch after these frm patches lands.

This revision was landed with ongoing or failed builds.Jul 13 2023, 12:34 AM
This revision was automatically updated to reflect the committed changes.