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

Thu Jan 27 06:13:33 PST 2022

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