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

[Craig Topper via Phabricator via cfe-commits]

craig.topper accepted this revision.
craig.topper 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/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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5475
 
+  if (IndexVT.getVectorElementType() == MVT::i64 && XLenVT == MVT::i32) {
+report_fatal_error("The V extension does not support EEW=64 for index "

craig.topper wrote:
> Can we truncate the index to nvxXi32 instead of erroring? Would that allow us 
> to preserve more test cases?
Done, thanks.



Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-masked-gather.ll:1033
 
-define <8 x i64> @mgather_baseidx_sext_v8i8_v8i64(i64* %base, <8 x i8> %idxs, 
<8 x i1> %m, <8 x i64> %passthru) {
-; RV32-LABEL: mgather_baseidx_sext_v8i8_v8i64:

craig.topper wrote:
> Can these test cases be preserved in an rv64 only test?
Done, thanks.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:5475
 
+  if (IndexVT.getVectorElementType() == MVT::i64 && XLenVT == MVT::i32) {
+report_fatal_error("The V extension does not support EEW=64 for index "

Can we truncate the index to nvxXi32 instead of erroring? Would that allow us 
to preserve more test cases?



Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-masked-gather.ll:1033
 
-define <8 x i64> @mgather_baseidx_sext_v8i8_v8i64(i64* %base, <8 x i8> %idxs, 
<8 x i1> %m, <8 x i64> %passthru) {
-; RV32-LABEL: mgather_baseidx_sext_v8i8_v8i64:

Can these test cases be preserved in an rv64 only test?


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added a comment.

ping


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added a comment.

Ping.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added inline comments.



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:179
const std::vector ,
-   StringRef RequiredExtension, unsigned NF);
+   const std::vector RequiredExtensions, unsigned NF);
   ~RVVIntrinsic() = default;

craig.topper wrote:
> RequiredExtensions should be a reference
Done, thanks.



Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:349
+  if (IndexLog2EEW == 6 && !Subtarget->is64Bit()) {
+errs() << "The V extension does not support EEW=64 for index values "
+  "when XLEN=32\n";

craig.topper wrote:
> This would just print a message to stderr but wouldn't fail the program. 
> Probably should use report_fatal_error. Or we could let the caller go to 
> SelectCode which would also trigger a "Cannot select" fatal error.
Done, thanks.



Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:430
+  if (IndexLog2EEW == 6 && !Subtarget->is64Bit()) {
+errs() << "The V extension does not support EEW=64 for index values "
+  "when XLEN=32\n";

craig.topper wrote:
> Same as above
Done, thanks.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:179
const std::vector ,
-   StringRef RequiredExtension, unsigned NF);
+   const std::vector RequiredExtensions, unsigned NF);
   ~RVVIntrinsic() = default;

RequiredExtensions should be a reference



Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:349
+  if (IndexLog2EEW == 6 && !Subtarget->is64Bit()) {
+errs() << "The V extension does not support EEW=64 for index values "
+  "when XLEN=32\n";

This would just print a message to stderr but wouldn't fail the program. 
Probably should use report_fatal_error. Or we could let the caller go to 
SelectCode which would also trigger a "Cannot select" fatal error.



Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:430
+  if (IndexLog2EEW == 6 && !Subtarget->is64Bit()) {
+errs() << "The V extension does not support EEW=64 for index values "
+  "when XLEN=32\n";

Same as above


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added a comment.

Ping.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added inline comments.



Comment at: clang/include/clang/Basic/riscv_vector.td:680
   foreach type = TypeList in {
-foreach eew_list = EEWList in {
+foreach eew_list = Xlen32EEWList in {
   defvar eew = eew_list[0];

HsiangKai wrote:
> There is no need to define `Xlen32EEWList`. You could use `EEWList[0-2]` for 
> the purpose.
Done, thank you.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai added inline comments.



Comment at: clang/include/clang/Basic/riscv_vector.td:680
   foreach type = TypeList in {
-foreach eew_list = EEWList in {
+foreach eew_list = Xlen32EEWList in {
   defvar eew = eew_list[0];

There is no need to define `Xlen32EEWList`. You could use `EEWList[0-2]` for 
the purpose.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added inline comments.



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

frasercrmck wrote:
> 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?
Yes, only for index load/store, we should add the macro to the generated header 
to make `EEW=64` just available on RV64.


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] 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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added inline comments.



Comment at: clang/include/clang/Basic/riscv_vector.td:693
+let Name = op # eew64 # "_v", IRName = op, IRNameMask = op # "_mask",
+RequiredExtensions = ["Xlen64"] in {
+def: RVVBuiltin<"v", "vPCe" # eew64_type # "Uv", type>;

jrtc27 wrote:
> Xlen64 is not an extension. Nor is RV64I, even, it is a base ISA, but that 
> would at least be somewhat defensible. In fact, Xlen64 would be parsed as a 
> valid non-standard extension called "Xlen" with major version 64 and minor 
> version 0, just like any other Xfoo.
So change Xlen64 to RV64 or create a new field of RVVBuiltin to describle it? 
Which one do you think is better?


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D106518#2895613 , @jacquesguan 
wrote:

> In D106518#2895445 , @craig.topper 
> wrote:
>
>> Why do they need to be disabled? Doesn’t the spec define them to truncate?
>
> In the 1.0-rc1, 18.2: The V extension supports all vector load and store 
> instructions (Section Vector Loads and Stores), except the V extension does 
> not support EEW=64 for index values when XLEN=32.
>
> I think this means that all index instruction with eew=64 is only supported 
> in RV64.

Thank you. Can you put that in the patch description so it gets into the commit 
log.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jianjian Guan via Phabricator via cfe-commits]

jacquesguan added a comment.

In D106518#2895445 , @craig.topper 
wrote:

> Why do they need to be disabled? Doesn’t the spec define them to truncate?

In the 1.0-rc1, 18.2: The V extension supports all vector load and store 
instructions (Section Vector Loads and Stores), except the V extension does not 
support EEW=64 for index values when XLEN=32.

I think this means that all index instruction with eew=64 is only supported in 
RV64.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



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

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.



Comment at: clang/include/clang/Basic/riscv_vector.td:693
+let Name = op # eew64 # "_v", IRName = op, IRNameMask = op # "_mask",
+RequiredExtensions = ["Xlen64"] in {
+def: RVVBuiltin<"v", "vPCe" # eew64_type # "Uv", type>;

Xlen64 is not an extension. Nor is RV64I, even, it is a base ISA, but that 
would at least be somewhat defensible. In fact, Xlen64 would be parsed as a 
valid non-standard extension called "Xlen" with major version 64 and minor 
version 0, just like any other Xfoo.


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] D106518: [RISCV] Disable EEW=64 for index values when XLEN=32.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Why do they need to be disabled? Doesn’t the spec define them to truncate?


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