[PATCH] D107290: [RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:101
+  } else {
+    RVVBitsMin = RVVVectorBitsMinOpt;
+    RVVBitsMax = RVVVectorBitsMaxOpt;
----------------
craig.topper wrote:
> frasercrmck wrote:
> > frasercrmck wrote:
> > > craig.topper wrote:
> > > > If clang always emits the attribute, are these options effectively dead for clang codegen?
> > > 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.
> > Well we now have `zvl` which kinda solve the "min" problem at the frontend level.
> > 
> > Thinking about it again, though, maybe it's not such a bad thing to have clang emit min=<zvl>, max=2^16/RVVBitsPerBlock and then allow backend codegen flags to override that. Then the onus is clearly on the user not to do anything wrong. We could assert if the user-provided values are clearly at odds with the attribute?
> I'm fine with that. I think we should consider dropping the riscv-v-vector-bits-min flag and just have a -riscv-v-fixed-width-vectorization-flag until we can prove that vectorization is robust. Bugs like D117663 make me nervous about blindly vectorizing code right now.
Yeah... I just realised that by taking `vscale_range` to mean `-riscv-v-vector-bits-min`, and us now using `zvl` to dictate `vscale_range`, we're effectively enabling fixed-length support by default now. I don't really want to introduce such a change in behaviour in this patch.

Maybe we should delay this patch until we have a `-riscv-v-fixed-width-vector-support` flag, or something, as you suggest. That or we emit `vscale_range` now but ignore it in the backend until such a change has been made.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:105
+      RVVBitsMax = RISCV::RVVVLENBitsMax;
+  }
+  // Allow user options to override these.
----------------
khchen wrote:
> For forward compatibility, if there is no VScaleRangeAttr, maybe we could initialize the RVVBitsMin as zvl*b if it is present?
> I guess maybe some exist IRs have zvl with no VScaleRangeAttr?
It's complicated due to us using `RVVBitsMin != 0` to also enable fixed-length vectorization. Defaulting that to our `zvl*b` extension is a change in behaviour. See the discussion with Craig above this one.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vscale-range.ll:162
+
+attributes #0 = { vscale_range(2,1024) }
+attributes #1 = { vscale_range(4,1024) }
----------------
khchen wrote:
> frasercrmck wrote:
> > khchen wrote:
> > > frasercrmck wrote:
> > > > khchen wrote:
> > > > > I'm thinking do we need to test zvl and vscale_range in the same attribute?
> > > > > ex. `attributes #0 = { vscale_range(2,1024) "target-features"="+zvl512b" }`
> > > > Perhaps yeah. Just to check - what exactly for? Because we need `zvl` in the attributes for correctness, or in order to test the combination of `zvl` architecture and `vscale_range` to test what happens when they disagree?
> > > Just test for they disagree.
> > > Do you know what's expected value for different `vscale_range` value in two function after function inlining? If they are always have the same minimum value for VLEN, I think we don't need a check.
> > Good idea.
> > 
> > As for inlining, I can't see anything that would //prevent// inlining of functions with different `vscale_range` attributes, per se. However, I was looking at `TTI::areInlineCompatible` and the default implementation checks whether CPU/Feature Strings are equivalent. The frontend should ensure that `vscale_range` attributes match up 1:1 with our `+zvl` feature strings so I think in practice we won't inline functions with different `zvl` values in clang-generated C/C++ code. But users could write IR with different `vscale_range` attributes and we'd happily inline them, which sounds fishy. What do you think?
> Thanks for investigation!!! 
> I think we can postpone this inline issue until we really need to fix it. 
> at least the function would keep the feature string, which may include zvl*b, right?
> 
> BTW, could you please try the C code in https://godbolt.org/z/6hfTaxTj5 to see what's `vscale_range` value for function `vadd256` and `vadd512`? Are they expected value?
> 
> 
Yeah the feature string looks to contain `zvl*b` as we expect -- in simple cases (see below). I've updated this test to check for them too.

Thanks for the example! I tried it. We have a couple of issues.

Firstly, the `vscale_range` is not correctly set for the functions. It is taken from whichever `zvl*b` we set on the command line. If I do `-target-feature +zvl128b` all functions have `vscale_range(2,1024)`, if I do `-target-feature +zvl256b` all functions have `(4,1024)`, etc. So something's not being communicated properly.

The second issue is that, because of this (I think) when using the non-CC1 driver, the subtarget initialization crashes if I compile with `-march=rv64gcv` or any `zvl*b` up to `-march=rv64gcv_zvl512b1p0` because the `-march` we specify there determines the `vscale_range` which in turn determines `RVVBitsMin`, but that's "lower than the Zvl*b limitation" so an assert triggers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107290/new/

https://reviews.llvm.org/D107290