khchen added inline comments.
================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:99
+ if (VScaleRangeAttr.isValid()) {
+ RVVBitsMin = VScaleRangeAttr.getVScaleRangeMin() * RISCV::RVVBitsPerBlock;
+ if (VScaleRangeAttr.getVScaleRangeMax().hasValue())
----------------
Could we have an assertion to prevent RVVBitsMin and Zvl are different?
================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:105
+ RVVBitsMax = RISCV::RVVVLENBitsMax;
+ }
+ // Allow user options to override these.
----------------
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?
================
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:
> > > > 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?
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
