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

[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] D142144: [RISCV][Driver] Add -mrvv-vector-bits= option similar to -msve-vector-bits=

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

LGTM other than test header comment that needs changed.




Comment at: clang/test/Driver/riscv-rvv-vector-bits.c:2
+// 
-
+// Tests for the -msve-vector-bits flag
+// 
-

Still need to adjust this comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142144

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142144: [RISCV][Driver] Add -mrvv-vector-bits= option similar to -msve-vector-bits=

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2150
+  Args.MakeArgString("-mvscale-min=" + llvm::Twine(VScaleMin)));
+} else if (!Val.equals("scalable")) {
+  // Handle the unsupported values passed to mrvv-vector-bits.

craig.topper wrote:
> frasercrmck wrote:
> > Is this check right? I don't see mention of "scalable" in the commit 
> > description. Should this be a plain `else`?
> "scalable" allows you to cancel the option appearing earlier on the command 
> line. Similar to why most bool options have a "no-" version. It's let you do 
> thing like append to CFLAGS to override a global setting in a make file.
Makes sense, thanks. I'm happy to include it as an option value, now that it's 
documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142144

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142144: [RISCV][Driver] Add -rvv-vector-bits= option similar to -sve-vector-bits=

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/include/clang/Driver/Options.td:3587
+  HelpText<"Specify the size in bits of an RVV vector register. Defaults to 
the"
+   " vector length agnostic value of \"scalable\". Also accepts 
\"zvl\""
+   " to use the value implied by -march/-mcpu. (RISC-V only)">;

Same question as below with the value `"scalable"`. Should we excise it for now?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2150
+  Args.MakeArgString("-mvscale-min=" + llvm::Twine(VScaleMin)));
+} else if (!Val.equals("scalable")) {
+  // Handle the unsupported values passed to mrvv-vector-bits.

Is this check right? I don't see mention of "scalable" in the commit 
description. Should this be a plain `else`?



Comment at: clang/test/Driver/riscv-rvv-vector-bits.c:2
+// 
-
+// Tests for the -msve-vector-bits flag
+// 
-

Need to adjust this comment



Comment at: clang/test/Driver/riscv-rvv-vector-bits.c:16
+// RUN: %clang -c %s -### -target riscv64-linux-gnu -march=rv64gc_zve64x \
+// RUN:  -mrvv-vector-bits=scalable 2>&1 | FileCheck 
--check-prefix=CHECK-SCALABLE %s
+

Another use of `"scalable"` which I'm not sure about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142144

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137516: [TargetSupport] Move TargetParser API in a separate LLVM component.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

In D137516#3911245 , @arsenm wrote:

> I think this is fine, but think the name needs to be bikeshedded. "Target" 
> and "Support" are already overloaded enough as it is. Is there anything else 
> that would really ever go into this library?

I was wondering this new library could house MVTs in some form. I've wanted to 
auto-generate both the C++ and tablegen MVTs from a single source of truth for 
a while.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137516

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116735: [RISCV] Adjust RV64I data layout by using n32:64 in layout string

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.

LGTM other than a nit, but I concur that a comment in AutoUpgrade would be nice.




Comment at: llvm/docs/ReleaseNotes.rst:123
+* i32 is now a native type in the datalayout string. This enables
+  LoopStrengthReduce for loops with i32 induction variables. And other
+  optimizations.

something like `, among other optimizations` to avoid full stop + "and"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116735

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116735: [RISCV] Adjust RISCV data layout by using n32:64 in layout string

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

I agree with @reames, though I do think the patch description could use a 
rewrite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116735

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D136106

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:55
+This means that the the following versions are now required to build LLVM
+and there is no way to supress this error.
+

`suppress`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Just nits from me.




Comment at: clang/include/clang/Sema/RISCVIntrinsicManager.h:9
+//
+// This file defines the RISCVIntrinsicManager, which handle RISC-V vector
+// intrinsic functions.

`handles`



Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:49
+  // Index of RISCVIntrinsicManagerImpl::IntrinsicList.
+  SmallVector Indexs;
+};

`Indices` (or `Indexes`)?



Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:218
+  for (int Log2LMUL = -3; Log2LMUL <= 3; Log2LMUL++) {
+if (!(Record.Log2LMULMask & (1 << (Log2LMUL + 3 {
+  continue;

Drop curly braces



Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:185-186
+   
Record.OverloadedSuffixSize);
+for (int TypeRangeMaskShift = 0;
+ TypeRangeMaskShift <= static_cast(BasicType::MaxOffset);
+ ++TypeRangeMaskShift) {

aaron.ballman wrote:
> Given that we're bit twiddling with this, I'd feel more comfortable if this 
> was `unsigned int` rather than `int` (same for `BaseTypeI`).
+1



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:47
+
+  // Required extensions for this intrinsic.
+  unsigned RequiredExtension;

Comment is plural, variable is singular.



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:111
+   std::vector *SemaRecords = nullptr);
+  /// Create all intrinsics record and SemaSignatureTable from SemaRecords.
+  void createRVVIntrinsicRecord(std::vector ,

all records, plural?



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:578
+
+if (SemaRecords) {
+  // Create SemaRecord

`if (!SemaRecords) continue;`? Might make things a little more readable.



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:637
+Out.emplace_back(Record);
+Out.back().Name = SR.Name.c_str();
+Out.back().OverloadedName = SR.OverloadedName.c_str();

I assume the compiler's able to avoid recomputing `Out.back()` multiple times? 
We could take a reference to `Out.back()` and use that, just in case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124062: [RISCV][Clang][NFC] Update vid intrinsic tests.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

Were they perhaps passing because `i64 [[VL:%.*]]` was matching the `, i64 0` 
too? Seems like a flaw in the checks generated by the script, but I can see how 
the majority of the time the brevity is nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124062

___
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

[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

[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

[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] D118253: [RISCV] Add the passthru operand for some RVV nomask unary and nullary intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118253

___
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

[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: [PoC][RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck updated this revision to Diff 402572.
frasercrmck added a comment.
Herald added a subscriber: pcwang-thead.

rebase
take minimum from zvl extensions
allow backend options to override attribute values
add extra testing


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> 

[PATCH] D112986: [Clang][RISCV] Restrict rvv builtin-s with zve macro-s

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

LGTM too. Though the commit title and message has hyphens in places I wouldn't 
expect them. `macros` and `builtins` is fine.




Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:797
 
   // Init RISC-V extensions
   for (const auto  : OutInTypes) {

nit: This still says "extensions"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112986

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.
Herald added subscribers: eopXD, VincentWu, luke957, 
achieveartificialintelligence.



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

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?


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] D117860: [RISCV] Remove experimental prefix from rvv-related extensions.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Thanks for the patch! Not sure the best way to review this. The tests are 
passing, which is a good sign. One option could be to split the "meaningful" 
changes into a separate diff for easier viewing? We probably don't need to see 
all the test RUN line changes, for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117860

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117647: [RISCV] Add destination operand for RVV nomask load intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Thanks, LGTM.

As a heads up, I've pinched the use of `_TU` as a suffix in D117561 
. The conflicts should be minor (one 
location) for whichever patch is merged second.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117647

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112987: [RISCV] Bump rvv-related extensions from 0.10 to 1.0

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

If we're bumping it to 1.0, does that mean it's no longer "experimental"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112987

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117647: [RISCV] Add destination operand for RVV nomask load intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Is "destination operand" the terminology we want? I'd have thought "passthru" 
was more conventional.




Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1194
 Operands.push_back(Node->getOperand(CurOp++));
+  if (Node->getOperand(CurOp).isUndef())
+CurOp++;

If I'm reading this right, isn't it possible that we could have a masked/tu 
intrinsic, so push back the passthru operand, increment `CurOp` then check 
whether the //pointer// operand is Undef, then wrongly increment `CurOp` a 
second time? So basically an undef base pointer has the potential to 
(presumably) crash the compiler somewhere later?

Don't we unconditionally want to increment `CurOp` (a single time) since we now 
always have a passthru operand that we want to skip over?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117647

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112613: [RISCV] Change TARGET_BUILTIN require to zve32x for vector instruction

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

LGTM but I don't have full scope on the builtins side of things. I'd also add a 
missing word to the title: "Change TARGET_BUILTIN //to// require ..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112613

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115709: [RISCV] Remove Vamo Extention

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

I think it'd be helpful for the description to note why this is being removed, 
what happened to the extension, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115709

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108694: [RISCV] Add the zvl extension according to the v1.0 spec

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/test/Preprocessor/riscv-target-features.c:230
+// RUN: | FileCheck --check-prefix=CHECK-V-MINVLEN %s
+// CHECK-V-MINVLEN: __riscv_v_min_vlen 128

Are we able to test non-default values of `__riscv_v_min_vlen` here?



Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:119
+  if (ZvlLen > RVVVectorBitsMax)
+return 0;
+  // FIXME: Change to >= 32 when VLEN = 32 is supported

Is this intuitive behaviour? If the user supplies `RVVVectorBitsMax` and it's 
less than `ZvlLen`, should it silently return? Or do we instead see 
`RVVVectorBitsMax` as a user-guided limit //on top// of the architecture? Which 
means it can be less but not more? I'm not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108694

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111062: [RISCV] Rename some assembler mnemonic and intrinsic functions for RVV 1.0.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: luke957.

Thanks @khchen, that's great. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111062: [RISCV] Rename some assembler mnemonic and intrinsic functions for RVV 1.0.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.
Herald added a reviewer: luke957.



Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vmand.c:97
 
 // CHECK-RV64-LABEL: @test_vmandnot_mm_b8(
+// CHECK-RV64-LABEL: @test_vmandn_mm_b8(

Was this test manually updated? I'm not sure how we could still expect to see 
`test_vmandnot_mm_b8`. Please use `update_cc_test_checks` if you haven't 
already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112534: [PoC][RISCV] Use an attribute to declare C intrinsics with different policy.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.
Herald added a reviewer: luke957.

Just nits from me at this stage.




Comment at: clang/include/clang/Basic/AttrDocs.td:2150
+  let Content = [{
+Users could use the attribute to specify the policy of destination tail and
+destination inactive masked-off elements in the vector operations. There are

Nit, but the use of `could` seems out of place in this documentation. Is `can` 
or `may` perhaps more common?



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:895
 ListSeparator LS;
 for (unsigned i = 0; i < InputTypes.size(); ++i)
   OS << LS << InputTypes[i]->getTypeStr();

This variable `i` shadowing the outer loop's induction variable is a little 
odd. Perhaps the outer loop could use a range-based for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112534

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112408: [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

I think the rest of my comments would be to do with `zvl` so I'll leave it 
there to avoid repetition.




Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:65
+  unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxEew() const { return MaxEew; }
+  unsigned getMaxEewFp() const { return MaxEewFp; }

Aside from the discussion about EEW vs. ELEN, something about the 
capitalization irks me. I realise we already have `XLen` but `Eew` looks... 
wrong. If other people disagree then that's fine.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:67
 {"zvlsseg", RISCVExtensionVersion{0, 10}},
+{"zvl32b", RISCVExtensionVersion{0, 10}},
+{"zvl64b", RISCVExtensionVersion{0, 10}},

Should this be in this patch? Or has some rebasing gone wrong and introduced 
code for D108694?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:733
+
+  {"zvl64b", {"zvl32b"}},
+  {"zvl128b", {"zvl64b"}},

Again, should `zvl` code be in this patch?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:765
+if (Implication != Implications.end()) {
+  for (auto ImpliedExtName : Implication->second) {
+if (WorkList.count(ImpliedExtName))

Really minor, but here you're using `auto` for `StringRef` but earlier and 
elsewhere it's `auto &`. I'm not sure which is preferred. Presumably 
`StringRef`s are cheap to copy and `auto` is fine? If `auto &` is more 
prominent in this file then go with that.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:787
 
+void RISCVISAInfo::updateMinVLen() {
+  for (auto  : Exts) {

`zvl` patch?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:468
+
+  if (Error Result = ISAInfo->checkDependency())
+return std::move(Result);

I'm not the most familiar with this API, but do we really need to 
`checkDependency` here when it's done in the next line?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:696
+
+  if (Error Result = ISAInfo->checkDependency())
+return std::move(Result);

Same here. Duplicate `checkDependency`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112408

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112398: [RISCV] Add ABI testing for Float16.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

It looks as though all checks are checking the same thing? Presumably this is 
expected? I wonder if allowing an extra combined check 
(`--check-prefixes=CHECK,CHECK-ZFH-ILP32F` or something) would make it more 
obvious when things *are* different between the different configs.

I'm not familiar with how `Float16` is supposed to behave if the target doesn't 
advertise support `zfh`, but I come more from OpenCL where it's either fully 
supported or "storage-only", in which case I wouldn't expect a `fadd` to get 
past the frontend (or maybe it'd enforce promotion to `float`?). This isn't 
necessarily a blocker - I'm just showing the limits of my knowledge in this 
area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112398

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112408: [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCV.td:182
 
+def FeatureExtZve32x
+: SubtargetFeature<"experimental-zve32x", "HasStdExtZve32x", "true",

craig.topper wrote:
> frasercrmck wrote:
> > Do we need to define distinct `SubtargetFeature`s for each of these 
> > extensions or could they be broken down into a single `MaxEEW` feature (32 
> > or 64) in conjunction with the pre-existing F/D features. This seems like 
> > it's more complicated than it needs to be.
> I don't think it is quite that simple. Couldn't you have a scalar D and have 
> zve64f vector?
Yes, that's fair. Though I still think we can create a more intuitive system of 
`Predicate`s to handle the TableGen aspects, as you've begun to do in D112496.



Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:141
+  // either v or zve* suppaort v instructions
+  bool hasStdExtV() const { return HasStdExtV || HasStdExtZve32x; }
+  bool hasStdExtZve32x() const { return HasStdExtZve32x; }

craig.topper wrote:
> frasercrmck wrote:
> > Is this correct? I thought we'd keep `hasStdExtV` as being the 
> > single-letter V extension, and Zve32x isn't that.
> I just put up D112496 to stop using hasStdExtV everywhere.
Ah right now I see what this was trying to do. I think your patch helps things, 
thanks.



Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll:158
 ; CHECK-NEXT:vmv.x.s a0, v8
+; CHECK-NEXT:lui a1, 1048560
+; CHECK-NEXT:or a0, a0, a1

craig.topper wrote:
> frasercrmck wrote:
> > What's going on here, do you know?
> I believe this is coming from the nan-boxing for f16 in 
> RISCVTargetLowering::splitValueIntoRegisterParts. The addition of +f must 
> have changed PartVT from i32/i64 to f32. Even though we're using i32 for the 
> return due to ABI.
Ah, interesting. I can't tell if that's a bug fix, i.e., if it's invalid to 
compile this test without `f` - though shouldn't we pass `experimental-zfh` by 
that same logic? Regardless, maybe we could split this off and pre-commit it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112408

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112408: [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Don't we need to teach ISel some tricks before we consider these extensions 
supported? E.g., we need to stop i64 vectors being legal under zve32x or zve32f.




Comment at: llvm/lib/Target/RISCV/RISCV.td:182
 
+def FeatureExtZve32x
+: SubtargetFeature<"experimental-zve32x", "HasStdExtZve32x", "true",

Do we need to define distinct `SubtargetFeature`s for each of these extensions 
or could they be broken down into a single `MaxEEW` feature (32 or 64) in 
conjunction with the pre-existing F/D features. This seems like it's more 
complicated than it needs to be.



Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:141
+  // either v or zve* suppaort v instructions
+  bool hasStdExtV() const { return HasStdExtV || HasStdExtZve32x; }
+  bool hasStdExtZve32x() const { return HasStdExtZve32x; }

Is this correct? I thought we'd keep `hasStdExtV` as being the single-letter V 
extension, and Zve32x isn't that.



Comment at: llvm/test/CodeGen/RISCV/attributes.ll:8
 ; RUN: llc -mtriple=riscv32 -mattr=+c %s -o - | FileCheck --check-prefix=RV32C 
%s
-; RUN: llc -mtriple=riscv32 
-mattr=+experimental-v,+experimental-zvamo,+experimental-zvlsseg %s -o - | 
FileCheck --check-prefix=RV32V %s
+; RUN: llc -mtriple=riscv32 
-mattr=+f,+d,+experimental-v,+experimental-zvamo,+experimental-zvlsseg %s -o - 
| FileCheck --check-prefix=RV32V %s
 ; RUN: llc -mtriple=riscv32 -mattr=+experimental-zfh %s -o - | FileCheck 
--check-prefix=RV32ZFH %s

Why is this being changed in this patch?



Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll:158
 ; CHECK-NEXT:vmv.x.s a0, v8
+; CHECK-NEXT:lui a1, 1048560
+; CHECK-NEXT:or a0, a0, a1

What's going on here, do you know?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112408

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112102: [RISCV] Reduce the number of RISCV vector builtins by an order of magnitude.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Minor typo in the description: `differnet`

Does this help with compile times, binary sizes, etc?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112102

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112102: [RISCV] Reduce the number of RISCV vector builtins by an order of magnitude.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/test/CodeGen/RISCV/rvv/vmerge-rv32.ll:1358
+; CHECK-NEXT:vsetvli zero, a0, e16, mf4, ta, mu
+; CHECK-NEXT:vmerge.vvm v8, v8, v9, v0
+; CHECK-NEXT:ret

HsiangKai wrote:
> vmerge.vvm is for integer vectors, doesn't it? Why does it work for floating 
> point vector types?
It's just a `vselect` in llvm terms, so doesn't care what its (vector) inputs 
are. The only reason we need `vfmerge` is when one operand is a scalar 
floating-point register.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112102

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105690: [RISCV] Rename assembler mnemonic of unordered floating-point reductions for v1.0-rc change

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

In D105690#3044417 , @HsiangKai wrote:

> I think we could restart to review this patch.

Thanks for bringing it up - I've lost track of the various 1.0 patches.

This one LGTM from what I can tell.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111062: [RISCV] Rename some assembler mnemonic and intrinsic functions for RVV 1.0.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

LGTM in general. My comments are all about comments. I know the old names are 
kept as aliases but I still think it's better to reference the "real" 
instructions where we can.




Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2468
 //  pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
 //  expansion: vmslt{u}.vx vt, va, x;  vmandnot.mm vd, vd, vt
 assert(Inst.getOperand(0).getReg() == RISCV::V0 &&

Comment here



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2486
 // pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
 // expansion: vmslt{u}.vx vt, va, x; vmandnot.mm vt, v0, vt; vmandnot.mm 
vd,
 // vd, v0; vmor.mm vd, vt, vd

Comment here too



Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:896
   // If the MaskedOff value and the Mask are the same value use
   // vmslt{u}.vx vt, va, x;  vmandnot.mm vd, vd, vt
   // This avoids needing to copy v0 to vd before starting the next 
sequence.

Comment needs updating



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4167
   case ISD::VP_REDUCE_AND: {
 // vpopc ~x == 0
 SDValue TrueMask = DAG.getNode(RISCVISD::VMSET_VL, DL, ContainerVT, VL);

Comment needs updating here and below



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4201
   // Note that we must return the start value when no elements are operated
   // upon. The vpopc instructions we've emitted in each case above will return
   // 0 for an inactive vector, and so we've already received the neutral value:

Stale mnemonic name here



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:266
 
   //  vpopc.m with additional mask and VL operands.
+  VCPOP_VL,

Nit: comment needs updating too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111062

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106044: [RISCV] Update to vlm.v and vsm.v according to v1.0-rc1.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.

LGTM in principle but `aliases.s` is apparently failing with something that 
looks relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106044

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93298: [RISCV] add the MC layer support of Zfinx extension

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

In D93298#3014160 , @jrtc27 wrote:

> The amount of duplication here really depresses me and is only going to get 
> worse once codegen is added, but TableGen isn't able to have operands that 
> use different register classes based on even HwMode, that I know of, and 
> whilst you could make use of multi classes to generate both versions of the 
> instructions you can't easily do that for patterns, so I don't know what the 
> right answer is.

Do we know if it's possible to teach TableGen how to do that? This extension 
seems like a good reason to implement such a feature as the duplication really 
is unfortunate.

I thought that `MCOperandInfo` may be an issue but there's already dynamic 
lookup there to support `isLookupPtrRegClass`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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


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: [PoC][RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:349
+  unsigned VLENMax = 65536;
+  return std::make_pair(VLENMin / 64, VLENMax / 64);
+}

craig.topper wrote:
> Should we move RVVBitsPerBlock to RISCVTargetParser.def? Or some other place 
> that can be shared between lllvm/lib/Target/RISCV/ and here?
Good idea. I also added the "StdV" min/max values of `128`/`65536` in there. 
However, I just put them in `TargetParser.h` as putting them in the `.def`  
file felt a bit odd and you had to account for preprocessor logic. It still 
feels a little odd but I agree that sharing these values is important. Other 
targets have specific values in there so it's not unprecedented. It is 
target-adjacent data, even if it's not (currently) dependent on triples or cpus.



Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:106
+  assert(RVVBitsMin % 128 == 0 &&
+ "RVV requires vector length in multiples of 128!");
+  assert(RVVBitsMax % 128 == 0 &&

kito-cheng wrote:
> RISC-V require VLEN in power of 2, multiples of 128 is constraint for SVE :p
> https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#2-implementation-defined-constant-parameters
Yeah to be honest I was just being cheeky/lazy here :) Since our current 
implementation requires `VLEN >= 128` we know that VLEN must always be a 
multiple of 128. But yes this isn't really the right way of coding it, even if 
it does the right thing. I've fixed that up now.


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: [PoC][RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck updated this revision to Diff 369416.
frasercrmck marked 2 inline comments as done.
frasercrmck added a comment.
Herald added a subscriber: dexonsmith.

- rebase
- move V VLEN bits-per-block (64), min (128), max (65536) defines into 
TargetParser.h
- clean up assertions


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

Index: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
===
--- llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
+++ llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
@@ -21,6 +21,7 @@
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/CodeGen/BasicTTIImpl.h"
 #include "llvm/IR/Function.h"
+#include "llvm/Support/TargetParser.h"
 
 namespace llvm {
 
Index: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
===
--- llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -32,6 +32,18 @@
 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
 
+static cl::opt RVVVectorBitsMaxOpt(
+"riscv-v-vector-bits-max",
+cl::desc("Assume V extension vector registers are at most this big, "
+ "with zero meaning no maximum size is assumed."),
+cl::init(0), cl::Hidden);
+
+static cl::opt RVVVectorBitsMinOpt(
+"riscv-v-vector-bits-min",
+cl::desc("Assume V extension vector registers are at least this big, "
+ "with zero meaning no minimum size is assumed."),
+cl::init(0), cl::Hidden);
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
   RegisterTargetMachine X(getTheRISCV32Target());
   RegisterTargetMachine Y(getTheRISCV64Target());
@@ -78,13 +90,58 @@
   Attribute TuneAttr = F.getFnAttribute("tune-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
+  unsigned RVVBitsMin = 0;
+  unsigned RVVBitsMax = 0;
+  Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange);
+  if (VScaleRangeAttr.isValid()) {
+std::tie(RVVBitsMin, RVVBitsMax) = VScaleRangeAttr.getVScaleRangeArgs();
+RVVBitsMin *= RISCV::RVVBitsPerBlock;
+RVVBitsMax *= RISCV::RVVBitsPerBlock;
+  } else {
+RVVBitsMin = RVVVectorBitsMinOpt;
+RVVBitsMax = RVVVectorBitsMaxOpt;
+  }
+
+  assert((RVVBitsMin == 0 || isPowerOf2_32(RVVBitsMin)) &&
+ "RVV requires vector length to be a power of two!");
+  assert((RVVBitsMax == 0 || isPowerOf2_32(RVVBitsMax)) &&
+ "RVV requires vector length to be a power of two!");
+  assert((RVVBitsMin == 0 || RVVBitsMin >= RISCV::StdVVLENBitsMin) &&
+ "RVV vector size must be no smaller than the minimum allowed by the "
+ "specification!");
+  assert(RVVBitsMax <= RISCV::StdVVLENBitsMax &&
+ "RVV vector size must be no larger than the maximum allowed by the "
+ "specification!");
+  assert((RVVBitsMax == 0 || RVVBitsMax >= RVVBitsMin) &&
+ "Minimum RVV vector size should not be larger than its maximum!");
+
+  // Sanitize user input in case of no asserts.
+  if (RVVBitsMax != 0)
+RVVBitsMin = std::min(RVVBitsMin, RVVBitsMax);
+  RVVBitsMin = PowerOf2Floor((RVVBitsMin < RISCV::StdVVLENBitsMin ||
+  RVVBitsMin > RISCV::StdVVLENBitsMax)
+ ? 0
+ : RVVBitsMin);
+
+  RVVBitsMax = std::max(RVVBitsMin, RVVBitsMax);
+  RVVBitsMax = PowerOf2Floor((RVVBitsMax < RISCV::StdVVLENBitsMin ||
+  RVVBitsMax > RISCV::StdVVLENBitsMax)
+ ? 0
+ : RVVBitsMax);
+
   std::string CPU =
   CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;
   std::string TuneCPU =
   TuneAttr.isValid() ? TuneAttr.getValueAsString().str() : CPU;
   std::string FS =
   FSAttr.isValid() ? FSAttr.getValueAsString().str() : TargetFS;
+
   std::string Key = CPU + TuneCPU + FS;
+  Key += "RVVMin";
+  Key += std::to_string(RVVBitsMin);
+  Key += "RVVMax";
+  Key += std::to_string(RVVBitsMax);
+
   auto  = SubtargetMap[Key];
   if (!I) {
 // This needs to be done before we create a new subtarget since any
@@ -101,7 +158,8 @@
   }
   ABIName = ModuleTargetABI->getString();
 }
-I = std::make_unique(TargetTriple, CPU, TuneCPU, FS, ABIName, *this);
+I = std::make_unique(
+TargetTriple, CPU, TuneCPU, FS, ABIName, RVVBitsMin, RVVBitsMax, *this);
   }
   return 

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

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Ah no, my mistake. This would be a drop in functionality if `getMaxVScale` is 
removed, since its replacement only checks the IR attribute and will not be 
affected by our backend flags.


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: [PoC][RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

This may be as far as we can take this patch without exposing RVV vectors bit 
control to the user/driver and having to worry about the concerns that spring 
from that: linking objects compiled with different RVV vector bits options, 
LTO, etc.

I believe that with the current state of the patch, the default, hard-coded 
`vscale_range` with values mandated by the spec, combined with the existing 
backend options for overrides, mean we're not losing any functionality.


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: [PoC][RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck updated this revision to Diff 367232.
frasercrmck added a comment.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

update usage in vein of AArch64:

- use vscale_range attribute to determine RVV vector bits min/max values
- if no attribute is present, use existing backend flags
- sanitize and pass RVV vector bits from RISCVTargetMachine through to 
RISCVSubtarget
- RISCVSubtarget just stores and reports


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-vscale-range.c
  llvm/lib/Target/RISCV/RISCVSubtarget.cpp
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

Index: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
===
--- llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -32,6 +32,18 @@
 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
 
+static cl::opt RVVVectorBitsMaxOpt(
+"riscv-v-vector-bits-max",
+cl::desc("Assume V extension vector registers are at most this big, "
+ "with zero meaning no maximum size is assumed."),
+cl::init(0), cl::Hidden);
+
+static cl::opt RVVVectorBitsMinOpt(
+"riscv-v-vector-bits-min",
+cl::desc("Assume V extension vector registers are at least this big, "
+ "with zero meaning no minimum size is assumed."),
+cl::init(0), cl::Hidden);
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
   RegisterTargetMachine X(getTheRISCV32Target());
   RegisterTargetMachine Y(getTheRISCV64Target());
@@ -78,13 +90,50 @@
   Attribute TuneAttr = F.getFnAttribute("tune-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
+  unsigned RVVBitsMin = 0;
+  unsigned RVVBitsMax = 0;
+  Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange);
+  if (VScaleRangeAttr.isValid()) {
+std::tie(RVVBitsMin, RVVBitsMax) = VScaleRangeAttr.getVScaleRangeArgs();
+RVVBitsMin *= RISCV::RVVBitsPerBlock;
+RVVBitsMax *= RISCV::RVVBitsPerBlock;
+  } else {
+RVVBitsMin = RVVVectorBitsMinOpt;
+RVVBitsMax = RVVVectorBitsMaxOpt;
+  }
+
+  assert(RVVBitsMin % 128 == 0 &&
+ "RVV requires vector length in multiples of 128!");
+  assert(RVVBitsMax % 128 == 0 &&
+ "RVV requires vector length in multiples of 128!");
+  assert(RVVBitsMax <= 65536 &&
+ "RVV vector size must be no larger than 65536!");
+  assert((RVVBitsMax >= RVVBitsMin || RVVBitsMax == 0) &&
+ "Minimum RVV vector size should not be larger than its maximum!");
+
+  // Sanitize user input in case of no asserts.
+  if (RVVBitsMax != 0)
+RVVBitsMin = std::min(RVVBitsMin, RVVBitsMax);
+  RVVBitsMin =
+  PowerOf2Floor((RVVBitsMin < 128 || RVVBitsMin > 65536) ? 0 : RVVBitsMin);
+
+  RVVBitsMax = std::max(RVVBitsMin, RVVBitsMax);
+  RVVBitsMax =
+  PowerOf2Floor((RVVBitsMax < 128 || RVVBitsMax > 65536) ? 0 : RVVBitsMax);
+
   std::string CPU =
   CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;
   std::string TuneCPU =
   TuneAttr.isValid() ? TuneAttr.getValueAsString().str() : CPU;
   std::string FS =
   FSAttr.isValid() ? FSAttr.getValueAsString().str() : TargetFS;
+
   std::string Key = CPU + TuneCPU + FS;
+  Key += "RVVMin";
+  Key += std::to_string(RVVBitsMin);
+  Key += "RVVMax";
+  Key += std::to_string(RVVBitsMax);
+
   auto  = SubtargetMap[Key];
   if (!I) {
 // This needs to be done before we create a new subtarget since any
@@ -101,7 +150,8 @@
   }
   ABIName = ModuleTargetABI->getString();
 }
-I = std::make_unique(TargetTriple, CPU, TuneCPU, FS, ABIName, *this);
+I = std::make_unique(
+TargetTriple, CPU, TuneCPU, FS, ABIName, RVVBitsMin, RVVBitsMax, *this);
   }
   return I.get();
 }
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -62,6 +62,8 @@
   bool EnableSaveRestore = false;
   unsigned XLen = 32;
   MVT XLenVT = MVT::i32;
+  unsigned RVVVectorBitsMin;
+  unsigned RVVVectorBitsMax;
   uint8_t MaxInterleaveFactor = 2;
   RISCVABI::ABI TargetABI = RISCVABI::ABI_Unknown;
   BitVector UserReservedRegister;
@@ -82,7 +84,8 @@
 public:
   // Initializes the data members to match that of the specified triple.
   RISCVSubtarget(const Triple , StringRef CPU, StringRef TuneCPU,
- StringRef FS, StringRef ABIName, const TargetMachine );
+ StringRef FS, StringRef ABIName, unsigned RVVVectorBitsMin,
+ unsigned RVVVectorLMULMax, const TargetMachine );
 
   // Parses features string setting specified subtarget options. The
   // definition of this function is 

[PATCH] D105690: [RISCV] Rename assembler mnemonic of unordered floating-point reductions for v1.0-rc change

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Sorry, I can't find if we wrote it down in some other patch -- and someone can 
correct me if I'm wrong -- but in one of the recent LLVM RISC-V sync-up calls 
we agreed that we'd skip v0.10-rc and move straight to supporting v1.0 when 
it's made final. So I think this patch will probably have to wait for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106939: [RISCV] If the maskedoff is vundefined(), use ta, ma for vsetvli.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

In D106939#2915784 , @HsiangKai wrote:

> In D106939#2915134 , @HsiangKai 
> wrote:
>
>> In D106939#2912807 , @frasercrmck 
>> wrote:
>>
>>> LGTM but there are test failures. Is that just a whole load of `mu->ma` 
>>> changes that have been omitted for a smaller diff?
>>
>> Updated test cases are put in https://reviews.llvm.org/D107022.
>
> Should I combine these two into one patch?

Hmm, unsure, the test changes are pretty much unreviewable given how large the 
diff would be. There's nothing unexpected in there, presumably?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106939

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck created this revision.
frasercrmck added reviewers: craig.topper, rogfer01, HsiangKai, evandro, 
arcbbb, khchen.
Herald added subscribers: vkmr, luismarques, apazos, sameer.abuasal, s.egerton, 
Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, 
edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, 
simoncook, johnrusso, rbar, asb.
frasercrmck requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

This patch begins the process of supporting the `vscale_range` attribute
for RVV. It implements it according to our supported v0.10 version of
the specification, as opposed to the imminent v1.0.

Most notably, this patch implements the attribute conservatively
according to the minimum and maximum values of VLEN according to the
specification. However, the backend can be given more information about
VLEN using the `-riscv-v-vector-bits-min` and `-riscv-v-vector-bits-max`
flags. This means that the API it aims to replace,
`TargetTransformInfo::getMaxVScale`, may still generate better code with
its better knowledge.

It is unclear whether we want to move those backend options up into the
frontend, whether we are able to allow the backend to infer all
information from the IR attribute, or whether we even want to do that;
that's a wider discussion.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107290

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/test/CodeGen/riscv-vscale-range.c


Index: clang/test/CodeGen/riscv-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vscale-range.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple riscv64 -target-feature +experimental-v -S 
-emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @func() #0
+// CHECK: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+void func() {}
Index: clang/lib/Basic/Targets/RISCV.h
===
--- clang/lib/Basic/Targets/RISCV.h
+++ clang/lib/Basic/Targets/RISCV.h
@@ -111,7 +111,11 @@
 DiagnosticsEngine ) override;
 
   bool hasExtIntType() const override { return true; }
+
+  Optional>
+  getVScaleRange(const LangOptions ) const override;
 };
+
 class LLVM_LIBRARY_VISIBILITY RISCV32TargetInfo : public RISCVTargetInfo {
 public:
   RISCV32TargetInfo(const llvm::Triple , const TargetOptions )
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -335,6 +335,20 @@
   return true;
 }
 
+Optional>
+RISCVTargetInfo::getVScaleRange(const LangOptions ) const {
+  if (!HasV)
+return None;
+  // VLEN is defined in v0.10 to be at least 128 bits and at most 65536 bits,
+  // and vscale is VLEN/64.
+  // FIXME: v1.0 removes the minimum value.
+  // FIXME: The backend can be told about the more specific minimum/maximum
+  // VLEN but the frontend can't access this information.
+  unsigned VLENMin = 128;
+  unsigned VLENMax = 65536;
+  return std::make_pair(VLENMin / 64, VLENMax / 64);
+}
+
 bool RISCV32TargetInfo::isValidCPUName(StringRef Name) const {
   return llvm::RISCV::checkCPUKind(llvm::RISCV::parseCPUKind(Name),
/*Is64Bit=*/false);


Index: clang/test/CodeGen/riscv-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vscale-range.c
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple riscv64 -target-feature +experimental-v -S -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: @func() #0
+// CHECK: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+void func() {}
Index: clang/lib/Basic/Targets/RISCV.h
===
--- clang/lib/Basic/Targets/RISCV.h
+++ clang/lib/Basic/Targets/RISCV.h
@@ -111,7 +111,11 @@
 DiagnosticsEngine ) override;
 
   bool hasExtIntType() const override { return true; }
+
+  Optional>
+  getVScaleRange(const LangOptions ) const override;
 };
+
 class LLVM_LIBRARY_VISIBILITY RISCV32TargetInfo : public RISCVTargetInfo {
 public:
   RISCV32TargetInfo(const llvm::Triple , const TargetOptions )
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -335,6 +335,20 @@
   return true;
 }
 
+Optional>
+RISCVTargetInfo::getVScaleRange(const LangOptions ) const {
+  if (!HasV)
+return None;
+  // VLEN is defined in v0.10 to be at least 128 bits and at most 65536 bits,
+  // and vscale is VLEN/64.
+  // FIXME: v1.0 removes the minimum value.
+  // FIXME: The backend can be told about the more specific minimum/maximum
+  // VLEN but the frontend can't access this 

[PATCH] D106277: [SVE] Remove the interface for getMaxVScale in favour of the IR attributes

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.h:100
+  Optional>
+  getVScaleRange(const LangOptions ) const;
+

This clang-tidy warning needs satisfied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106277: [SVE] Remove the interface for getMaxVScale in favour of the IR attributes

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

In D106277#2913136 , @paulwalker-arm 
wrote:

> @craig.topper can you share RISCV plans around supporting vscale_range?  In 
> essence we'd like to deprecate the TTI method and have LLVM IR contain all 
> relevant information when is comes to interpreting vscale.
>
> Currently the usage is minimal and so checking both interfaces is not too bad 
> but they'll come a point when there's no TTI available and then only the side 
> supporting vscale_range can be considered.  There's also the LTO side of 
> things where relying on opt/llc flags to set register widths becomes fragile.

As it happens I was playing around with adding support for that today at least 
to get us started. I was going to put something up for review based on this 
patch, assuming this gets merged first.

However in the TTI method we're currently making use of some `RISCVSubtarget` 
properties to help refine the vscale range, so we'll need to think about how 
we'll deal with those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106939: [RISCV] If the maskedoff is vundefined(), use ta, ma for vsetvli.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

LGTM but there are test failures. Is that just a whole load of `mu->ma` changes 
that have been omitted for a smaller diff?




Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:379
   bool ForceTailAgnostic = RISCVII::doesForceTailAgnostic(TSFlags);
+  bool MaskAgnostic = true;
   bool TailAgnostic = true;

nit, but purely from a code layout and comment perspective this `MaskAgnostic` 
is in the middle of two "Tail" variables. Also the "tail" variables are 
well-commented ahead of time but the "mask" logic is only commented on inside 
the if statement below. It just looks a bit imbalanced.

I'm wondering if it'd be better there was even something simple like "Default 
to mask agnostic unless the operation is masked and the destination is tied to 
a source."



Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:409
   InstrInfo.setVTYPE(VLMul, SEW, /*TailAgnostic*/ TailAgnostic,
- /*MaskAgnostic*/ false, MaskRegOp);
+ /*MaskAgnostic*/ MaskAgnostic, MaskRegOp);
 

nit: maybe we don't need `/*TailAgnostic*/` or `/*MaskAgnostic*/` anymore?



Comment at: llvm/test/CodeGen/RISCV/rvv/maskedoff-undef.ll:3
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-v -verify-machineinstrs \
+; RUN:   --riscv-no-aliases < %s | FileCheck %s
+

I don't think we're typically using `--riscv-no-aliases` in our CodeGen tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106939

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:127
+
+bool RISCVISAInfo::isSupportedExtensionFeature(StringRef Ext) {
+  bool IsExperimental = stripExperimentalPrefix(Ext);

This looks like a `find_if` if that'd make it any simpler.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:147
+unsigned MinorVersion) {
+  for (auto const  :
+   filterSupportedExtensionInfosByName(Ext)) {

This also looks like a `find_if`



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:231
+  size_t RHSLen = RHS.length();
+  int LHSRank, RHSRank;
+  if (LHSLen == 1 && RHSLen != 1)

Not sure why these need to be declared up here.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:310
+errc::invalid_argument,
+"Failed to parsing major version number for extension '" + Ext + "'");
+

`Failed to parse ...`



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:315
+errc::invalid_argument,
+"Failed to parsing minor version number for extension '" + Ext + "'");
+

`Failed to parse`



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:359
+}
+// return true;
+return Error::success();

Commented-out code



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:414
+  // RISC-V ISA strings must be lowercase.
+  if (llvm::any_of(Arch, [](char C) { return isupper(C); })) {
+return createStringError(errc::invalid_argument,

I think you can just have `if (llvm::any_of(Arch, isupper))` here.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2036
+for (auto Feature : RISCVFeatureKV) {
+  if (llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key)) {
+clearFeatureBits(Feature.Value, Feature.Key);

Don't need `{}` here



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2055
+
+for (auto Feature : RISCVFeatureKV) {
+  if (ISAInfo.hasExtension(Feature.Key)) {

Don't need `{}` here either


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/include/clang/Basic/riscv_vector.td:555
 
+defvar Xlen32EEWList = [["8", "(Log2EEW:3)"],
+["16", "(Log2EEW:4)"],

jrtc27 wrote:
> Ignoring whether the change is actually correct, this should be capitalised 
> as XLen32EEWList, but really this should actually be RV32 not XLen32 as 
> that's not a term we use.
While we're here I'm wondering whether a top-level 
`Xlen32EEWList`/`RV32EEWList` is conveying the wrong thing. It's only the loads 
and stores that have a different EEW list on RV32, isn't it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106518

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105690: [RISCV] Rename assembler mnemonic of unordered floating-point reductions for v1.0-rc change

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:10
 /// This file describes the RISC-V instructions from the standard 'V' Vector
 /// extension, version 0.10.
 /// This version is still experimental as the 'V' extension hasn't been

khchen wrote:
> jacquesguan wrote:
> > khchen wrote:
> > > Do we need to update 0.10 to 1.0-rc?
> > > If the answer is yes, I think maybe we also need to update the clang part 
> > > (ex. arch parsing, predefine macro) in follow-up patches.
> > > 
> > > 
> > Maybe update it after finishing all changes in 1.0-rc?
> > Maybe update it after finishing all changes in 1.0-rc?
> Yes.
> 
> The other questions like how do you encode `rc1` in `march` or predefined 
> architecture extension macro.
> or maybe we could just use 1.0 directly because v is still an experiential 
> extension.
> 
> @luismarques  @frasercrmck @craig.topper @HsiangKai What do you think?
Maybe we can discuss in the call today, but my initial thoughts would be to 
just say 1.0 for the reasons you specified.

Perhaps there's already precedent in dealing with release-candidate specs for 
the base ISA or other extensions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106049: [RISCV] Update to vfredusum.vs and vfwredusum.vs according to v1.0-rc1.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Duplicate of D105690 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105690: [RISCV] Rename assembler mnemonic of unordered floating-point reductions for v1.0-rc change

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/test/CodeGen/RISCV/rvv/vfredusum-rv32.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-v,+d,+experimental-zfh 
-verify-machineinstrs \

Just to check - was this renaming done with `git mv`? Phabricator suggests that 
`vfredsum-rv32.ll` was deleted and this was added, which would be worse for the 
git history. It might be a phabricator quirk, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105690

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104822: [RISCV] Add vget/vset intrinsics for inserting and extracting between different lmuls.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

What's supposed to happen if the provided index is invalid? I'm suspecting we'd 
currently get a IR verification error on the insert/extract indices. I'm 
wondering if we can/should catch that earlier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103603: [Sema][RISCV] Allow ?: to select Typedef BuiltinType in C

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

In D103603#2795899 , @kito-cheng 
wrote:

> Testcase for AArch64/SVE:
>
>   #include 
>   
>   svint8_t a();
>   __SVInt8_t b();
>   
>   svint8_t foo(int cond){
> return cond ? a(): b();
>   }

Could that AArch64 test also be part of this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103603

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100819: [RISCV] Implement the vneg.v builtin.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1048
+   return Cmp < 0;
+ // Some mask intrinsics use the same IRName as unmasked.
+ // Sort the unmasked intrinsics first.

Does this only affect `vneg` or are there other changes to the tablegenned code 
as a consequence?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100819

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100611: [RISCV] Add new attribute __clang_riscv_builtin_alias for intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

LGTM. Anything else would be wondering if it can be merged/genericised with Arm 
somehow but that's not a blocker.




Comment at: clang/include/clang/Basic/AttrDocs.td:2141
+in RVV builtins, and still be recognized as clang builtins equivalent to the
+underlying name. For example, ``riscv_vector.h`` declares the function 
``vget`` with
+``__attribute__((__clang_riscv_builtin_alias(__builtin_rvv_vget_i8mf8x2_i8mf8)))``.

HsiangKai wrote:
> frasercrmck wrote:
> > Is `vget` real? I can't see it in `riscv_vector.h`
> Yes, it is for Zvlsseg. I have not upstreamed it. I use `vadd` instead in the 
> description.
Ah, great. I think `vadd`'s a good example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100611: [RISCV] Add new attribute __clang_riscv_builtin_alias for intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:2141
+in RVV builtins, and still be recognized as clang builtins equivalent to the
+underlying name. For example, ``riscv_vector.h`` declares the function 
``vget`` with
+``__attribute__((__clang_riscv_builtin_alias(__builtin_rvv_vget_i8mf8x2_i8mf8)))``.

Is `vget` real? I can't see it in `riscv_vector.h`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100611: [RISCV] Add new attribute __clang_riscv_builtin_alias for intrinsics.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Is this idea that this will later be "automatically" applied to builtins in 
`riscv_vector.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99984: [RISCV] Prevent __builtin_riscv_orc_b_64 from being compiled RV32 target.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:259
   .Case("riscv64", Is64Bit)
+  .Case("64bit", Is64Bit)
   .Case("m", HasM)

craig.topper wrote:
> frasercrmck wrote:
> > What's the difference between this new feature and `riscv64` above?
> The name I added to initFeatureMap has to match the feature name in RISCV.td 
> because it will end up in the target-features attribute in IR.
> 
> SemaChecking just calls hasFeature, but CodeGenFunction also checks builtin 
> features using the feature map. We check features in SemaChecking because the 
> CodeGenFunction diagnostic isn’t phrased well for RISCV.
Ah I see, so since it has to match our `"64bit"` `SubtargetFeature` we can't 
reuse `"riscv64"`? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99984: [RISCV] Prevent __builtin_riscv_orc_b_64 from being compiled RV32 target.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:259
   .Case("riscv64", Is64Bit)
+  .Case("64bit", Is64Bit)
   .Case("m", HasM)

What's the difference between this new feature and `riscv64` above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99984

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99482: [PoC][Clang][CodeGen] Do not use getelementptr for scalable struct.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

Are there any tests for this?




Comment at: clang/lib/CodeGen/CGCall.cpp:1299
  bool DestIsVolatile) {
   // Prefer scalar stores to first-class aggregate stores.
   if (llvm::StructType *STy = dyn_cast(Val->getType())) {

This comment might need to be sunk as it doesn't apply to structs containing 
scalable vectors?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99482

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99590: [Clang] Do not use memcpy for scalable struct copy.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/include/clang/AST/Type.h:4627
+  /// Recursively check all fields in the record for sizeless. If any field
+  /// is sizeless type, return true. Otherwise, return false.
+  bool hasSizelessFields() const;

nit: missing "a": `is a sizeless type`



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:2085
+  llvm::Value *SrcVec = Builder.CreateLoad(SrcPtr);
+  llvm::Value *DestVec = llvm::UndefValue::get(ConvertType(Ty));
+  llvm::Value *Vec;

Are `SrcVec` and `DestVec` the right names here? It took me a while to realise 
they're not vector types.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:2086
+  llvm::Value *DestVec = llvm::UndefValue::get(ConvertType(Ty));
+  llvm::Value *Vec;
+  for (unsigned I = 0;

We could define `Vec` inside the loop?



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:2088
+  for (unsigned I = 0;
+   I < getContext().getASTRecordLayout(Record).getFieldCount(); ++I) {
+Vec = Builder.CreateExtractValue(SrcVec, I);

I don't know if `getContext().getASTRecordLayout(Record).getFieldCount()` is 
expensive to compute, but this may be recomputing it on every iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99590

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99158: [RISCV][WIP] Implement intrinsics for P extension

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:715-732
+if (Subtarget.is64Bit()) {
+  addTypeForP(MVT::v8i8, MVT::i64);
+  addTypeForP(MVT::v4i16, MVT::i64);
+  addTypeForP(MVT::v2i32, MVT::i64);
+} else {
+  addTypeForP(MVT::v4i8, MVT::i32);
+  addTypeForP(MVT::v2i16, MVT::i32);

jrtc27 wrote:
> This seems like it will interact poorly with V if both are present
Indeed. I don't think we've really thought about how both of these could 
co-exist in llvm. I'm not sure they can. We don't have the context to decide 
upon how to lower an operation (to V or P) since we're just given the type.

Do we have to error if both are enabled simultaneously? Maybe if P only expects 
to be lowered via intrinsics we can fudge it. It's not pretty though. I came 
across this issue in a previous architecture where we had a v2i32 (or 
something) able to be lowered both via "scalar" and "vector" instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99158

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95680: [RISCV] Update the version number to v0.10 for vector.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95680

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95002: [RISCV] Update B extension version to 0.93.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoB.td:732
 // There's no encoding for roli in the current version of the 'B' extension
 // (v0.92) as it can be implemented with rori by negating the immediate.
 let Predicates = [HasStdExtZbbOrZbp] in {

This might need updated. The statement is still true in 0.92, but it's not the 
"current version"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94617: [RISCV] Add Zba feature and move add.uw and slli.uw to it.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94617

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90928: [OpenCL] Check for extension string extension lookup

[Fraser Cormack via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7793db35ca2c: [OpenCL] Check for extension string extension 
lookup (authored by erik2020, committed by frasercrmck).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90928

Files:
  clang/include/clang/Basic/OpenCLOptions.h


Index: clang/include/clang/Basic/OpenCLOptions.h
===
--- clang/include/clang/Basic/OpenCLOptions.h
+++ clang/include/clang/Basic/OpenCLOptions.h
@@ -32,38 +32,71 @@
   };
   llvm::StringMap OptMap;
 public:
+  /// Check if \c Ext is a recognized OpenCL extension.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \returns \c true if \c Ext is known, \c false otherwise.
   bool isKnown(llvm::StringRef Ext) const {
 return OptMap.find(Ext) != OptMap.end();
   }
 
+  /// Check if \c Ext is an enabled OpenCL extension.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \returns \c true if \c Ext is known and enabled, \c false otherwise.
   bool isEnabled(llvm::StringRef Ext) const {
-return OptMap.find(Ext)->second.Enabled;
+auto E = OptMap.find(Ext);
+return E != OptMap.end() && E->second.Enabled;
   }
 
-  // Is supported as either an extension or an (optional) core feature for
-  // OpenCL version \p CLVer.
+  /// Check if \c Ext is supported as either an extension or an (optional) core
+  /// feature for the given OpenCL version.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \param LO - \c LangOptions specifying the OpenCL version.
+  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
   bool isSupported(llvm::StringRef Ext, const LangOptions ) const {
+auto E = OptMap.find(Ext);
+if (E == OptMap.end()) {
+  return false;
+}
 // In C++ mode all extensions should work at least as in v2.0.
 auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
-auto I = OptMap.find(Ext)->getValue();
+auto I = E->getValue();
 return I.Supported && I.Avail <= CLVer;
   }
 
-  // Is supported (optional) OpenCL core features for OpenCL version \p CLVer.
-  // For supported extension, return false.
+  /// Check if \c Ext is supported as an (optional) OpenCL core features for
+  /// the given OpenCL version.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \param LO - \c LangOptions specifying the OpenCL version.
+  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
   bool isSupportedCore(llvm::StringRef Ext, const LangOptions ) const {
+auto E = OptMap.find(Ext);
+if (E == OptMap.end()) {
+  return false;
+}
 // In C++ mode all extensions should work at least as in v2.0.
 auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
-auto I = OptMap.find(Ext)->getValue();
+auto I = E->getValue();
 return I.Supported && I.Avail <= CLVer && I.Core != ~0U && CLVer >= I.Core;
   }
 
-  // Is supported OpenCL extension for OpenCL version \p CLVer.
-  // For supported (optional) core feature, return false.
+  /// Check if \c Ext is a supported OpenCL extension for the given OpenCL
+  /// version.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \param LO - \c LangOptions specifying the OpenCL version.
+  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
   bool isSupportedExtension(llvm::StringRef Ext, const LangOptions ) const {
+auto E = OptMap.find(Ext);
+if (E == OptMap.end()) {
+  return false;
+}
 // In C++ mode all extensions should work at least as in v2.0.
 auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
-auto I = OptMap.find(Ext)->getValue();
+auto I = E->getValue();
 return I.Supported && I.Avail <= CLVer && (I.Core == ~0U || CLVer < 
I.Core);
   }
 


Index: clang/include/clang/Basic/OpenCLOptions.h
===
--- clang/include/clang/Basic/OpenCLOptions.h
+++ clang/include/clang/Basic/OpenCLOptions.h
@@ -32,38 +32,71 @@
   };
   llvm::StringMap OptMap;
 public:
+  /// Check if \c Ext is a recognized OpenCL extension.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \returns \c true if \c Ext is known, \c false otherwise.
   bool isKnown(llvm::StringRef Ext) const {
 return OptMap.find(Ext) != OptMap.end();
   }
 
+  /// Check if \c Ext is an enabled OpenCL extension.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \returns \c true if \c Ext is known and enabled, \c false otherwise.
   bool isEnabled(llvm::StringRef Ext) const {
-return OptMap.find(Ext)->second.Enabled;
+auto E = OptMap.find(Ext);
+return E != OptMap.end() && E->second.Enabled;
   }
 
-  // Is supported as either an extension or an (optional) core feature for
-  // OpenCL version \p CLVer.
+  /// Check if \c Ext is supported as either an extension or an (optional) core
+  ///