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

2023-02-06 Thread Fraser Cormack via Phabricator via cfe-commits 
frasercrmck abandoned this revision.
frasercrmck added a comment.
Herald added a subscriber: luke.

Superseded by D139873  amongst others


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

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

2022-10-13 Thread Philip Reames via Phabricator via cfe-commits 
reames added a comment.
Herald added subscribers: sunshaoce, StephenFan, arichardson.
Herald added a project: All.

Not knowing about this patch, I posted D135894 
 which addresses a small sub-set of this.  If 
that goes in, I plan to iterative split off some other parts of this into stand 
alone changes.


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

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

2022-01-29 Thread Zakk Chen via Phabricator via cfe-commits 
khchen added inline comments.



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:
> > > > 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?
> > 
> > 
> Yeah the feature string looks to contain `zvl*b` as we expect -- in simple 
> cases (see below). I've updated this test to check for them too.
> 
> Thanks for the example! I tried it. We have a couple of issues.
> 
> Firstly, the `vscale_range` is not correctly set for the functions. It is 
> taken from whichever `zvl*b` we set on the command line. If I do 
> `-target-feature +zvl128b` all functions have `vscale_range(2,1024)`, if I do 
> `-target-feature +zvl256b` all functions have `(4,1024)`, etc. So something's 
> not being communicated properly.
> 
> The second issue is that, because of this (I think) when using the non-CC1 
> driver, the subtarget initialization crashes if I compile with 
> `-march=rv64gcv` or any `zvl*b` up to `-march=rv64gcv_zvl512b1p0` because the 
> `-march` we specify there determines the `vscale_range` which in turn 
> determines `RVVBitsMin`, but that's "lower than the Zvl*b limitation" so an 
> assert triggers.
Sorry, I have no idea about what's good way to fix them, or maybe RISC-V has 
not already supported ifunc then we could ignore this example, I'm not sure.

BTW, I'm wondering why we want to support `vscale_range` attribute in RISC-V V.
Could we get any benefit after supporting it? 
It seems like SVE does not have a way to encode vector length information, so 
it must introduce a new function attribute `vscale_range` in IR.
But in RISC-V V, we already have zvl*b target-feature to get the minimum vlen 
information, and the maximum vlen is always 65536. In addition, we also have 
default implication rule for zvl*b depend on V/Zve*.

It seem like we are trying to support users's manually IRs which have 
`vscale_range` without zvl*b target-feature, is it?  
Or am I misunderstanding the intention?


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

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

2022-01-28 Thread Fraser Cormack via Phabricator via cfe-commits 
frasercrmck added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:101
+  } else {
+RVVBitsMin = RVVVectorBitsMinOpt;
+RVVBitsMax = RVVVectorBitsMaxOpt;

craig.topper wrote:
> frasercrmck wrote:
> > frasercrmck wrote:
> > > craig.topper wrote:
> > > > If clang always emits the attribute, are these options effectively dead 
> > > > for clang codegen?
> > > Yes, that's a good point - I'd missed that. I'm not sure the best way of 
> > > keeping that ability apart from moving the options up to clang and 
> > > dealing with the fallout from that. Which I'm not even sure we //can// 
> > > deal with yet?
> > > 
> > > Unless we make the options override the attribute, though that might be 
> > > its own can of worms.
> > Well we now have `zvl` which kinda solve the "min" problem at the frontend 
> > level.
> > 
> > Thinking about it again, though, maybe it's not such a bad thing to have 
> > clang emit min=, max=2^16/RVVBitsPerBlock and then allow backend 
> > codegen flags to override that. Then the onus is clearly on the user not to 
> > do anything wrong. We could assert if the user-provided values are clearly 
> > at odds with the attribute?
> I'm fine with that. I think we should consider dropping the 
> riscv-v-vector-bits-min flag and just have a 
> -riscv-v-fixed-width-vectorization-flag until we can prove that vectorization 
> is robust. Bugs like D117663 make me nervous about blindly vectorizing code 
> right now.
Yeah... I just realised that by taking `vscale_range` to mean 
`-riscv-v-vector-bits-min`, and us now using `zvl` to dictate `vscale_range`, 
we're effectively enabling fixed-length support by default now. I don't really 
want to introduce such a change in behaviour in this patch.

Maybe we should delay this patch until we have a 
`-riscv-v-fixed-width-vector-support` flag, or something, as you suggest. That 
or we emit `vscale_range` now but ignore it in the backend until such a change 
has been made.



Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:105
+  RVVBitsMax = RISCV::RVVVLENBitsMax;
+  }
+  // Allow user options to override these.

khchen wrote:
> 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?
It's complicated due to us using `RVVBitsMin != 0` to also enable fixed-length 
vectorization. Defaulting that to our `zvl*b` extension is a change in 
behaviour. See the discussion with Craig above this one.



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:
> > > 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?
> 
> 
Yeah the feature string looks to contain `zvl*b` as we expect -- in simple 
cases (see below). I've updated this test to check for them too.

Thanks for the example! I tried it. We have a couple of issues.

Firstly, the `vscale_range` is not correctly set for the functions. It is taken 
from whichever

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

2022-01-28 Thread Fraser Cormack via Phabricator via cfe-commits 
frasercrmck updated this revision to Diff 404016.
frasercrmck added a comment.

rebase
check for zvl feature strings alongside vscale_range


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107290/new/

https://reviews.llvm.org/D107290

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/test/CodeGen/RISCV/riscv-vscale-range.c
  llvm/include/llvm/Support/TargetParser.h
  llvm/lib/Target/RISCV/RISCVISelLowering.cpp
  llvm/lib/Target/RISCV/RISCVISelLowering.h
  llvm/lib/Target/RISCV/RISCVSubtarget.cpp
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
  llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
  llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vscale-range.ll

Index: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vscale-range.ll
===
--- /dev/null
+++ llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vscale-range.ll
@@ -0,0 +1,167 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+v,+m -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=riscv64 -mattr=+v,+m -verify-machineinstrs < %s | FileCheck %s
+
+define <512 x i8> @vadd_v512i8_zvl128(<512 x i8> %a, <512 x i8> %b) #0 {
+; CHECK-LABEL: vadd_v512i8_zvl128:
+; CHECK:   # %bb.0:
+; CHECK-NEXT:addi sp, sp, -16
+; CHECK-NEXT:.cfi_def_cfa_offset 16
+; CHECK-NEXT:csrr a2, vlenb
+; CHECK-NEXT:li a4, 40
+; CHECK-NEXT:mul a2, a2, a4
+; CHECK-NEXT:sub sp, sp, a2
+; CHECK-NEXT:csrr a2, vlenb
+; CHECK-NEXT:li a4, 24
+; CHECK-NEXT:mul a2, a2, a4
+; CHECK-NEXT:add a2, sp, a2
+; CHECK-NEXT:addi a2, a2, 16
+; CHECK-NEXT:vs8r.v v16, (a2) # Unknown-size Folded Spill
+; CHECK-NEXT:csrr a2, vlenb
+; CHECK-NEXT:slli a2, a2, 5
+; CHECK-NEXT:add a2, sp, a2
+; CHECK-NEXT:addi a2, a2, 16
+; CHECK-NEXT:vs8r.v v8, (a2) # Unknown-size Folded Spill
+; CHECK-NEXT:li a2, 128
+; CHECK-NEXT:vsetvli zero, a2, e8, m8, ta, mu
+; CHECK-NEXT:addi a2, a3, 128
+; CHECK-NEXT:addi a4, a3, 384
+; CHECK-NEXT:vle8.v v8, (a4)
+; CHECK-NEXT:csrr a4, vlenb
+; CHECK-NEXT:slli a4, a4, 4
+; CHECK-NEXT:add a4, sp, a4
+; CHECK-NEXT:addi a4, a4, 16
+; CHECK-NEXT:vs8r.v v8, (a4) # Unknown-size Folded Spill
+; CHECK-NEXT:addi a4, a1, 128
+; CHECK-NEXT:vle8.v v8, (a1)
+; CHECK-NEXT:csrr a1, vlenb
+; CHECK-NEXT:slli a1, a1, 3
+; CHECK-NEXT:add a1, sp, a1
+; CHECK-NEXT:addi a1, a1, 16
+; CHECK-NEXT:vs8r.v v8, (a1) # Unknown-size Folded Spill
+; CHECK-NEXT:addi a1, a3, 256
+; CHECK-NEXT:vle8.v v8, (a1)
+; CHECK-NEXT:vle8.v v16, (a4)
+; CHECK-NEXT:vle8.v v24, (a2)
+; CHECK-NEXT:vle8.v v0, (a3)
+; CHECK-NEXT:addi a1, sp, 16
+; CHECK-NEXT:vs8r.v v0, (a1) # Unknown-size Folded Spill
+; CHECK-NEXT:csrr a1, vlenb
+; CHECK-NEXT:slli a1, a1, 3
+; CHECK-NEXT:add a1, sp, a1
+; CHECK-NEXT:addi a1, a1, 16
+; CHECK-NEXT:vl8re8.v v0, (a1) # Unknown-size Folded Reload
+; CHECK-NEXT:vadd.vv v8, v0, v8
+; CHECK-NEXT:csrr a1, vlenb
+; CHECK-NEXT:slli a1, a1, 3
+; CHECK-NEXT:add a1, sp, a1
+; CHECK-NEXT:addi a1, a1, 16
+; CHECK-NEXT:vs8r.v v8, (a1) # Unknown-size Folded Spill
+; CHECK-NEXT:csrr a1, vlenb
+; CHECK-NEXT:slli a1, a1, 4
+; CHECK-NEXT:add a1, sp, a1
+; CHECK-NEXT:addi a1, a1, 16
+; CHECK-NEXT:vl8re8.v v8, (a1) # Unknown-size Folded Reload
+; CHECK-NEXT:vadd.vv v16, v16, v8
+; CHECK-NEXT:csrr a1, vlenb
+; CHECK-NEXT:li a2, 24
+; CHECK-NEXT:mul a1, a1, a2
+; CHECK-NEXT:add a1, sp, a1
+; CHECK-NEXT:addi a1, a1, 16
+; CHECK-NEXT:vl8re8.v v8, (a1) # Unknown-size Folded Reload
+; CHECK-NEXT:vadd.vv v8, v8, v24
+; CHECK-NEXT:csrr a1, vlenb
+; CHECK-NEXT:slli a1, a1, 5
+; CHECK-NEXT:add a1, sp, a1
+; CHECK-NEXT:addi a1, a1, 16
+; CHECK-NEXT:vl8re8.v v24, (a1) # Unknown-size Folded Reload
+; CHECK-NEXT:addi a1, sp, 16
+; CHECK-NEXT:vl8re8.v v0, (a1) # Unknown-size Folded Reload
+; CHECK-NEXT:vadd.vv v0, v24, v0
+; CHECK-NEXT:vse8.v v0, (a0)
+; CHECK-NEXT:addi a1, a0, 384
+; CHECK-NEXT:vse8.v v16, (a1)
+; CHECK-NEXT:addi a1, a0, 256
+; CHECK-NEXT:csrr a2, vlenb
+; CHECK-NEXT:slli a2, a2, 3
+; CHECK-NEXT:add a2, sp, a2
+; CHECK-NEXT:addi a2, a2, 16
+; CHECK-NEXT:vl8re8.v v16, (a2) # Unknown-size Folded Reload
+; CHECK-NEXT:vse8.v v16, (a1)
+; CHECK-NEXT:addi a0, a0, 128
+; CHECK-NEXT:vse8.v v8, (a0)
+; CHECK-NEXT:csrr a0, vlenb
+; CHECK-NEXT:li a1, 40
+; CHECK-NEXT:mul a0, a0, a1
+; CHECK-NEXT:add sp, sp, a0
+; CHECK-NEXT:addi sp, sp, 16
+; CHECK-NEXT:ret
+  %c = add <512 x i8> %a, %b
+  ret <512 x i8> %c
+}
+
+define <512 x i8> @vadd_v512i8_zvl256(<512 x i8> %a, <512 x i8> %b) #1 {
+; CHECK-LABEL: vadd_v512i8_zvl256:
+; CHECK:

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

2022-01-27 Thread Zakk Chen via Phabricator via cfe-commits 
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

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

2022-01-27 Thread Fraser Cormack via Phabricator via cfe-commits 
frasercrmck added a comment.

In 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-01-25 Thread Zakk Chen via Phabricator via cfe-commits 
khchen added inline comments.



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:
> > 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.


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

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

2022-01-25 Thread Paul Walker via Phabricator via cfe-commits 
paulwalker-arm added a comment.

Does this mean `RISCVTTIImpl::getMaxVScale()` can be removed?


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

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

2022-01-25 Thread Fraser Cormack via Phabricator via cfe-commits 
frasercrmck added inline comments.



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:
> 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?


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

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

2022-01-24 Thread Zakk Chen via Phabricator via cfe-commits 
khchen added inline comments.



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) }

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" }`


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