This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Rename assembler mnemonic of unordered floating-point reductions for v1.0-rc change
ClosedPublic

Authored by jacquesguan on Jul 9 2021, 3:19 AM.

Details

Summary

Rename vfredsum and vfwredsum to vfredusum and vfwredusum. Add aliases for vfredsum and vfwredsum.

Diff Detail

Event Timeline

jacquesguan created this revision.Jul 9 2021, 3:19 AM
jacquesguan requested review of this revision.Jul 9 2021, 3:19 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 9 2021, 3:19 AM

Fix llc test cases.

frasercrmck added inline comments.Jul 13 2021, 2:28 AM
llvm/test/CodeGen/RISCV/rvv/vfredusum-rv32.ll
1 ↗(On Diff #357488)

Just to check - was this renaming done with git mv? Phabricator suggests that vfredsum-rv32.ll was deleted and this was added, which would be worse for the git history. It might be a phabricator quirk, though.

luismarques added inline comments.Jul 13 2021, 2:47 AM
llvm/test/CodeGen/RISCV/rvv/vfredusum-rv32.ll
1 ↗(On Diff #357488)

AFAIK git mv doesn't do anything in particular to track renames. File renames are automatically detected based on the added and removed content, which means that if there are also changes to the content that detection might fail, and here the instruction renames did cause a lot of changes.

HsiangKai added inline comments.Jul 15 2021, 5:43 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1342

How about to set the Emit to 0 to lower the printing priority.

def : InstAlias<"vfredsum.vs $vd, $vs2, $vs1$vm",
                (VFREDUSUM_VS VR:$vd, VR:$vs2, VR:$vs1, VMaskOp:$vm), 0>;
1356

Ditto.

khchen added inline comments.Jul 15 2021, 6:48 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
10

Do we need to update 0.10 to 1.0-rc?
If the answer is yes, I think maybe we also need to update the clang part (ex. arch parsing, predefine macro) in follow-up patches.

jacquesguan added inline comments.Jul 15 2021, 7:22 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
10

Maybe update it after finishing all changes in 1.0-rc?

Set the Emit of InstAlias to 0.

khchen added inline comments.Jul 15 2021, 9:44 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
10

Maybe update it after finishing all changes in 1.0-rc?

Yes.

The other questions like how do you encode rc1 in march or predefined architecture extension macro.
or maybe we could just use 1.0 directly because v is still an experiential extension.

@luismarques @frasercrmck @craig.topper @HsiangKai What do you think?

frasercrmck added inline comments.Jul 22 2021, 2:10 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
10

Maybe we can discuss in the call today, but my initial thoughts would be to just say 1.0 for the reasons you specified.

Perhaps there's already precedent in dealing with release-candidate specs for the base ISA or other extensions?

kito-cheng added inline comments.Jul 22 2021, 6:57 AM
llvm/test/MC/RISCV/rvv/aliases.s
86

I guess you want to verify vfredsum.vs and vfwredsum.vs here?

HsiangKai added inline comments.Jul 22 2021, 7:32 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
10

I expect v1.0-rc1 will have no big change, especially for these renaming issues. I vote for 1.0 for the experimental extension.

jacquesguan added inline comments.Jul 22 2021, 6:35 PM
llvm/test/MC/RISCV/rvv/aliases.s
86

Yes, but because the Emit of the InstAlias is set to 0, the printing priority of alias is lower than instruction, It just print the mnemonic of the instruction.

craig.topper added inline comments.Jul 22 2021, 6:54 PM
llvm/test/MC/RISCV/rvv/aliases.s
86

Don't you want to use the alias vfredsum.vs as input and verify you get the non-alias vfredusum.vs back?

Fix alias test.

jacquesguan added inline comments.Jul 22 2021, 7:43 PM
llvm/test/MC/RISCV/rvv/aliases.s
86

Yes, you are right, I do not notice using wrong input, thank you.

Sorry, I can't find if we wrote it down in some other patch -- and someone can correct me if I'm wrong -- but in one of the recent LLVM RISC-V sync-up calls we agreed that we'd skip v0.10-rc and move straight to supporting v1.0 when it's made final. So I think this patch will probably have to wait for that.

I think we could restart to review this patch.

I think we could restart to review this patch.

Thanks for bringing it up - I've lost track of the various 1.0 patches.

This one LGTM from what I can tell.

This revision is now accepted and ready to land.Oct 6 2021, 9:23 AM

Rebase main.

Format code.

This revision was landed with ongoing or failed builds.Oct 11 2021, 11:57 PM
This revision was automatically updated to reflect the committed changes.

This patch as committed, deleted 5 test files instead of renaming them. I'm working on restoring them.

This patch as committed, deleted 5 test files instead of renaming them. I'm working on restoring them.

Deleted tests have been restored by 670c72f6f70434500d1475e1524a7088814fbc73

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-reduction-fp.ll