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