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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
