frasercrmck added a comment.
In D107290#3268949 <https://reviews.llvm.org/D107290#3268949>, @paulwalker-arm
wrote:
> Does this mean `RISCVTTIImpl::getMaxVScale()` can be removed?
Good question. I'm unsure at this stage. At hinted at in the description,
`getMaxVScale` can make use of backend-specific flags to hone the maximum down
a bit, whereas relying on the attribute basically reduces us to the one value
which the frontend will ever likely produce. So as it stands, the
`vscale_range` attribute is not at feature parity with this TTI method. I think
we'd have to come to a decision that this outcome is okay.
================
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:
> > > 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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107290/new/
https://reviews.llvm.org/D107290
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
