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