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

Zakk Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 27 10:05:15 PST 2022


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



More information about the llvm-commits mailing list