This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Rename some assembler mnemonic and intrinsic functions for RVV 1.0.
ClosedPublic

Authored by khchen on Oct 4 2021, 8:17 AM.

Details

Summary

Rename vpopc/vmandnot/vmornot to vcpop/vmandn/vmorn assembler mnemonic.

Diff Detail

Event Timeline

khchen created this revision.Oct 4 2021, 8:17 AM
khchen requested review of this revision.Oct 4 2021, 8:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 4 2021, 8:17 AM

LGTM in general. My comments are all about comments. I know the old names are kept as aliases but I still think it's better to reference the "real" instructions where we can.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2380

Comment here

2398

Comment here too

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
899

Comment needs updating

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4192

Comment needs updating here and below

4226

Stale mnemonic name here

llvm/lib/Target/RISCV/RISCVISelLowering.h
265–266

Nit: comment needs updating too

craig.topper added inline comments.Oct 6 2021, 9:35 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1383

Probably not worth having a blank line between vmandnot and vmornot

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

To be consistent with line 4683

// 16.2. Vector count population in mask vcpop.m
llvm/test/MC/RISCV/rvv/aliases.s
82

line "a2, v4, v0.t" up with the previous line.

85

Same here

88

And here

khchen updated this revision to Diff 378912.Oct 12 2021, 1:10 AM
khchen marked 8 inline comments as done.

address frasercrmck and Craig's comments.

jrtc27 added inline comments.Oct 12 2021, 9:07 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1385

Ugh, spec mandating aliases for the pre-ratified names... this is just going to encourage people who don't know any better to use the old names and you'll never be able to kill it off :(

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-mask-logic.ll
4

Revert

(how do you even manage that, the NOTE line is autogenerated...)

I think this looks good other than the whitespace in fixed-vectors-mask-logic.ll that Jessica commented on.

khchen updated this revision to Diff 380001.Oct 15 2021, 7:28 AM

address @jrtc27's comment.

In fact I don't know why I modified the Note line..

khchen marked 5 inline comments as done.Oct 15 2021, 7:28 AM
jrtc27 added inline comments.Oct 15 2021, 7:36 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1383

This still applies

khchen updated this revision to Diff 380007.Oct 15 2021, 7:47 AM

removed blank

asb added a comment.Oct 28 2021, 3:35 AM

Is this blocked on anything or is it good to land?

frasercrmck added inline comments.Oct 28 2021, 6:37 AM
clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vmand.c
97

Was this test manually updated? I'm not sure how we could still expect to see test_vmandnot_mm_b8. Please use update_cc_test_checks if you haven't already.

luke957 resigned from this revision.Oct 28 2021, 7:30 AM

So sorry for my bad herald script.

khchen updated this revision to Diff 383219.Oct 28 2021, 6:52 PM

Updated all tests by script update_cc_test_checks and update_llc_test_checks.

@frasercrmck Yes, I used sed to update tests and I don't know why I had unexpected result...
Thanks for you double check!

frasercrmck accepted this revision.Oct 29 2021, 2:00 AM

Thanks @khchen, that's great. LGTM.

This revision is now accepted and ready to land.Oct 29 2021, 2:00 AM
llvm/test/CodeGen/RISCV/rvv/vmorn-rv64.ll