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

[Zakk Chen via Phabricator via cfe-commits]

khchen added inline comments.


================
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) }
----------------
frasercrmck wrote:
> 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.
Sorry, I have no idea about what's good way to fix them, or maybe RISC-V has not already supported ifunc then we could ignore this example, I'm not sure.

BTW, I'm wondering why we want to support `vscale_range` attribute in RISC-V V.
Could we get any benefit after supporting it? 
It seems like SVE does not have a way to encode vector length information, so it must introduce a new function attribute `vscale_range` in IR.
But in RISC-V V, we already have zvl*b target-feature to get the minimum vlen information, and the maximum vlen is always 65536. In addition, we also have default implication rule for zvl*b depend on V/Zve*.

It seem like we are trying to support users's manually IRs which have `vscale_range` without zvl*b target-feature, is it?  
Or am I misunderstanding the intention?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107290