Page MenuHomePhabricator

[PoC][RISCV] Add support for the vscale_range attribute
Needs ReviewPublic

Authored by frasercrmck on Aug 2 2021, 9:52 AM.

Details

Summary

This patch begins the process of supporting the vscale_range attribute
for RVV. It implements it according to our supported v0.10 version of
the specification, as opposed to the imminent v1.0.

Most notably, this patch implements the attribute conservatively
according to the minimum and maximum values of VLEN according to the
specification. However, the backend can be given more information about
VLEN using the -riscv-v-vector-bits-min and -riscv-v-vector-bits-max
flags. This means that the API it aims to replace,
TargetTransformInfo::getMaxVScale, may still generate better code with
its better knowledge.

It is unclear whether we want to move those backend options up into the
frontend, whether we are able to allow the backend to infer all
information from the IR attribute, or whether we even want to do that;
that's a wider discussion.

Diff Detail

Event Timeline

frasercrmck created this revision.Aug 2 2021, 9:52 AM
frasercrmck requested review of this revision.Aug 2 2021, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2021, 9:52 AM

update usage in vein of AArch64:

  • use vscale_range attribute to determine RVV vector bits min/max values
  • if no attribute is present, use existing backend flags
  • sanitize and pass RVV vector bits from RISCVTargetMachine through to RISCVSubtarget
  • RISCVSubtarget just stores and reports
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2021, 9:20 AM
Matt added a subscriber: Matt.Aug 18 2021, 1:59 PM

This may be as far as we can take this patch without exposing RVV vectors bit control to the user/driver and having to worry about the concerns that spring from that: linking objects compiled with different RVV vector bits options, LTO, etc.

I believe that with the current state of the patch, the default, hard-coded vscale_range with values mandated by the spec, combined with the existing backend options for overrides, mean we're not losing any functionality.

Ah no, my mistake. This would be a drop in functionality if getMaxVScale is removed, since its replacement only checks the IR attribute and will not be affected by our backend flags.

craig.topper added inline comments.Aug 20 2021, 11:57 AM
clang/lib/Basic/Targets/RISCV.cpp
349

Should we move RVVBitsPerBlock to RISCVTargetParser.def? Or some other place that can be shared between lllvm/lib/Target/RISCV/ and here?

kito-cheng added inline comments.Aug 25 2021, 7:02 PM
llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
106

RISC-V require VLEN in power of 2, multiples of 128 is constraint for SVE :p
https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#2-implementation-defined-constant-parameters

frasercrmck marked 2 inline comments as done.
  • rebase
  • move V VLEN bits-per-block (64), min (128), max (65536) defines into TargetParser.h
  • clean up assertions
frasercrmck added inline comments.Aug 30 2021, 4:57 AM
clang/lib/Basic/Targets/RISCV.cpp
349

Good idea. I also added the "StdV" min/max values of 128/65536 in there. However, I just put them in TargetParser.h as putting them in the .def file felt a bit odd and you had to account for preprocessor logic. It still feels a little odd but I agree that sharing these values is important. Other targets have specific values in there so it's not unprecedented. It is target-adjacent data, even if it's not (currently) dependent on triples or cpus.

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
106

Yeah to be honest I was just being cheeky/lazy here :) Since our current implementation requires VLEN >= 128 we know that VLEN must always be a multiple of 128. But yes this isn't really the right way of coding it, even if it does the right thing. I've fixed that up now.

craig.topper added inline comments.Aug 30 2021, 6:16 PM
llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
101

If clang always emits the attribute, are these options effectively dead for clang codegen?

frasercrmck added inline comments.Aug 31 2021, 7:15 AM
llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
101

Yes, that's a good point - I'd missed that. I'm not sure the best way of keeping that ability apart from moving the options up to clang and dealing with the fallout from that. Which I'm not even sure we can deal with yet?

Unless we make the options override the attribute, though that might be its own can of worms.