This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Disable EEW=64 for index values when XLEN=32.
ClosedPublic

Authored by jacquesguan on Jul 21 2021, 8:44 PM.

Details

Summary

According to the 1.0-rc1, 18.2 : The V extension supports all vector load and store instructions (Section Vector Loads and Stores), except the V extension does not support EEW=64 for index values when XLEN=32, disable EEW=64 for vector index load/store when XLEN=32.

Diff Detail

Event Timeline

jacquesguan created this revision.Jul 21 2021, 8:44 PM
jacquesguan requested review of this revision.Jul 21 2021, 8:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 21 2021, 8:44 PM

Why do they need to be disabled? Doesn’t the spec define them to truncate?

jrtc27 added inline comments.Jul 21 2021, 8:57 PM
clang/include/clang/Basic/riscv_vector.td
571

Ignoring whether the change is actually correct, this should be capitalised as XLen32EEWList, but really this should actually be RV32 not XLen32 as that's not a term we use.

723

Xlen64 is not an extension. Nor is RV64I, even, it is a base ISA, but that would at least be somewhat defensible. In fact, Xlen64 would be parsed as a valid non-standard extension called "Xlen" with major version 64 and minor version 0, just like any other Xfoo.

Why do they need to be disabled? Doesn’t the spec define them to truncate?

In the 1.0-rc1, 18.2: The V extension supports all vector load and store instructions (Section Vector Loads and Stores), except the V extension does not support EEW=64 for index values when XLEN=32.

I think this means that all index instruction with eew=64 is only supported in RV64.

Why do they need to be disabled? Doesn’t the spec define them to truncate?

In the 1.0-rc1, 18.2: The V extension supports all vector load and store instructions (Section Vector Loads and Stores), except the V extension does not support EEW=64 for index values when XLEN=32.

I think this means that all index instruction with eew=64 is only supported in RV64.

Thank you. Can you put that in the patch description so it gets into the commit log.

jacquesguan edited the summary of this revision. (Show Details)Jul 21 2021, 11:40 PM
jacquesguan added inline comments.Jul 21 2021, 11:53 PM
clang/include/clang/Basic/riscv_vector.td
723

So change Xlen64 to RV64 or create a new field of RVVBuiltin to describle it? Which one do you think is better?

frasercrmck added inline comments.Jul 22 2021, 3:30 AM
clang/include/clang/Basic/riscv_vector.td
571

While we're here I'm wondering whether a top-level Xlen32EEWList/RV32EEWList is conveying the wrong thing. It's only the loads and stores that have a different EEW list on RV32, isn't it?

jacquesguan added inline comments.Jul 22 2021, 4:07 AM
clang/include/clang/Basic/riscv_vector.td
571

Yes, only for index load/store, we should add the macro to the generated header to make EEW=64 just available on RV64.

HsiangKai added inline comments.Jul 22 2021, 9:00 AM
clang/include/clang/Basic/riscv_vector.td
710

There is no need to define Xlen32EEWList. You could use EEWList[0-2] for the purpose.

remove Xlen32EEWList and rename Xlen64 to RV64.

clang/include/clang/Basic/riscv_vector.td
710

Done, thank you.

craig.topper added inline comments.Aug 1 2021, 7:55 PM
clang/utils/TableGen/RISCVVEmitter.cpp
178

RequiredExtensions should be a reference

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

This would just print a message to stderr but wouldn't fail the program. Probably should use report_fatal_error. Or we could let the caller go to SelectCode which would also trigger a "Cannot select" fatal error.

483

Same as above

Replace errs with report_fatal_error and change the vector argument type to reference type.

jacquesguan added inline comments.Aug 1 2021, 8:37 PM
clang/utils/TableGen/RISCVVEmitter.cpp
178

Done, thanks.

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

Done, thanks.

483

Done, thanks.

Truncate i64 index vector for vector gather/scatter in RV32 to avoid using index load instruction that has EEW=64.

remove newlines in error string.

rebase main.

rebase main.

craig.topper added inline comments.Jan 6 2022, 8:57 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5541

Can we truncate the index to nvxXi32 instead of erroring? Would that allow us to preserve more test cases?

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-masked-gather.ll
1033

Can these test cases be preserved in an rv64 only test?

Truncate the index and update test.

jacquesguan added inline comments.Jan 7 2022, 12:19 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5541

Done, thanks.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-masked-gather.ll
1033

Done, thanks.

This revision is now accepted and ready to land.Jan 7 2022, 9:50 AM

Fix comment.

This revision was landed with ongoing or failed builds.Jan 9 2022, 6:52 PM
This revision was automatically updated to reflect the committed changes.