Page MenuHomePhabricator

[RISCV] Make v extension imply zvamo, zvlsseg
Changes PlannedPublic

Authored by simoncook on Jan 21 2021, 9:19 AM.

Details

Summary

The V (vector) extension should enable the zvamo and zvlsseg extensions,
but currently is defined the other way around in TableGen. This updates
the SubtargetFeatures to reverse the inheritence order to be correct.

Diff Detail

Event Timeline

simoncook created this revision.Jan 21 2021, 9:19 AM
simoncook requested review of this revision.Jan 21 2021, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 9:19 AM

Doesn't this mean that if you only enable zvlsseg, you'll be able to use the instruction in that extension but not the vsetvli instruction that you need to program the VL register?

Doesn't this mean that if you only enable zvlsseg, you'll be able to use the instruction in that extension but not the vsetvli instruction that you need to program the VL register?

@craig.topper To be honest, I'm not at all familiar with the v extension or any of the zv* extensions. I wrote this in response to D94931 which says that V should imply zv* but not the other way around. Looking back now at D85069 it looks there was some discussion suggesting which way around it should be, so it could be this patch is unnecessary and breaks stuff. Do you have any better understand of the vector spec as to which way round is correct?

@kito-cheng Similarly, do you know if the correct order of implication/requirements is well defined somewhere. If there are gcc/binutils patches what does it do here?

Doesn't this mean that if you only enable zvlsseg, you'll be able to use the instruction in that extension but not the vsetvli instruction that you need to program the VL register?

@craig.topper To be honest, I'm not at all familiar with the v extension or any of the zv* extensions. I wrote this in response to D94931 which says that V should imply zv* but not the other way around. Looking back now at D85069 it looks there was some discussion suggesting which way around it should be, so it could be this patch is unnecessary and breaks stuff. Do you have any better understand of the vector spec as to which way round is correct?

@kito-cheng Similarly, do you know if the correct order of implication/requirements is well defined somewhere. If there are gcc/binutils patches what does it do here?

Hmmmmm, I don't think the spec has well defined that, it's kind of vague in this part, I've argue with ISA guys for this, and then got this change in spec.

So how about GCC/binutils, GNU toolchain using a weird work-around here, v implied zvamo and zvlsseg, and zvamo/zvlsseg/zvqmacc implied v...

@craig.topper that's good point, I create a issue on spec: https://github.com/riscv/riscv-v-spec/issues/626

So how about GCC/binutils, GNU toolchain using a weird work-around here, v implied zvamo and zvlsseg, and zvamo/zvlsseg/zvqmacc implied v...

Having the implications both ways round is a bit annoying to implement this way. I can't forward declare SubtargetFeatures in TableGen, and presumably even if I could I would send the tools in an infinite loop trying to initialise things in this case, so a more detailed/careful change would be needed.

In the mean time I will park this change, and add 'v' implying its subfeatures in clang, since this would catch most cases we expect users to use (I don't think manually enabling features is a common use case). I can then later look at a more detailed/complete fix when enabling features directly with -mattr.

@kito-cheng To clarify, if someone asks for e.g -march=rv32izvamo0p9, I enable 'v' because that's implied by zvamo, do we also enable zvlsseg, or does asking just for zvamo mean 'v+zvamo' but not 'v+zvamo+zvlsseg'/do I imply these features recursively?

In the mean time I will park this change, and add 'v' implying its subfeatures in clang, since this would catch most cases we expect users to use (I don't think manually enabling features is a common use case). I can then later look at a more detailed/complete fix when enabling features directly with -mattr.

That's SGTM.

@kito-cheng To clarify, if someone asks for e.g -march=rv32izvamo0p9, I enable 'v' because that's implied by zvamo, do we also enable zvlsseg, or does asking just for zvamo mean 'v+zvamo' but not 'v+zvamo+zvlsseg'/do I imply these features recursively?

In current GCC implementation, yes, -march=rv32izvamo will result -march=rv32iv_zvamo_zvlsseg, it's buggy work-around to me, but currently seems like no good choice here.

Related issue on v-spec: https://github.com/riscv/riscv-v-spec/issues/546

simoncook planned changes to this revision.Jan 24 2021, 2:10 PM