[PATCH] D133239: [RISCV][MC] Add minimal support for Ztso extension

[Philip Reames via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa4a29438f451: [RISCV][MC] Add minimal support for Ztso 
extension (authored by reames).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133239

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/attribute-arch.s
  llvm/test/MC/RISCV/elf-flags.s

Index: llvm/test/MC/RISCV/elf-flags.s
===
--- llvm/test/MC/RISCV/elf-flags.s
+++ llvm/test/MC/RISCV/elf-flags.s
@@ -5,6 +5,8 @@
 # RUN: llvm-mc -triple=riscv32 -mattr=+e -filetype=obj < %s \
 # RUN:   | llvm-readobj --file-headers - \
 # RUN:   | FileCheck -check-prefix=CHECK-RVE %s
+# RUN: llvm-mc -triple=riscv32 -mattr=+experimental-ztso -filetype=obj < %s | llvm-readobj --file-headers - | FileCheck -check-prefixes=CHECK-TSO %s
+# RUN: llvm-mc -triple=riscv64 -mattr=+experimental-ztso -filetype=obj < %s | llvm-readobj --file-headers - | FileCheck -check-prefixes=CHECK-TSO %s
 
 # CHECK-RVI:   Flags [ (0x0)
 # CHECK-RVI-NEXT:  ]
@@ -17,4 +19,8 @@
 # CHECK-RVE-NEXT: EF_RISCV_RVE (0x8)
 # CHECK-RVE-NEXT:   ]
 
+# CHECK-TSO:Flags [ (0x10)
+# CHECK-NEXT-TSO  EF_RISCV_TSO (0x10)
+# CHECK-NEXT-TSO]
+
 nop
Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -196,3 +196,6 @@
 
 .attribute arch, "rv32izca0p70"
 # CHECK: attribute  5, "rv32i2p0_zca0p70"
+
+.attribute arch, "rv32iztso0p1"
+# CHECK: attribute  5, "rv32i2p0_ztso0p1"
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -84,6 +84,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbom %s -o - | FileCheck --check-prefix=RV64ZICBOM %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicboz %s -o - | FileCheck --check-prefix=RV64ZICBOZ %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
 ; RV32M: .attribute 5, "rv32i2p0_m2p0"
 ; RV32ZMMUL: .attribute 5, "rv32i2p0_zmmul1p0"
@@ -170,6 +171,7 @@
 ; RV64ZICBOM: .attribute 5, "rv64i2p0_zicbom1p0"
 ; RV64ZICBOZ: .attribute 5, "rv64i2p0_zicboz1p0"
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
+; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
   %1 = add i32 %a, 1
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -92,6 +92,7 @@
   bool HasStdExtZicboz = false;
   bool HasStdExtZicbop = false;
   bool HasStdExtZmmul = false;
+  bool HasStdExtZtso = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -192,6 +193,7 @@
   bool hasStdExtZicboz() const { return HasStdExtZicboz; }
   bool hasStdExtZicbop() const { return HasStdExtZicbop; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
+  bool hasStdExtZtso() const { return HasStdExtZtso; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCV.td
===
--- llvm/lib/Target/RISCV/RISCV.td
+++ llvm/lib/Target/RISCV/RISCV.td
@@ -454,6 +454,13 @@
 AssemblerPredicate<(all_of FeatureStdExtZicbop),
 "'Zicbop' (Cache-Block Prefetch Instructions)">;
 
+def FeatureStdExtZtso
+: SubtargetFeature<"experimental-ztso", "HasStdExtZtso", "true",
+   "'Ztso' (Memory Model - Total Store Order)">;
+def HasStdExtZtso : Predicate<"Subtarget->hasStdExtZTso()">,
+   AssemblerPredicate<(all_of FeatureStdExtZtso),
+   "'Ztso' (Memory Model - Total Store Order)">;
+
 // Feature32Bit exists to mark CPUs that support RV32 to distinquish them from
 // tuning CPU names.
 def Feature32Bit
Index: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
===
--- llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -157,6 +157,8 @@
 
   if (Features[RISCV::FeatureStdExtC])
 EFlags |= ELF::EF_RISCV_RVC;
+  if 

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames created this revision.
reames added reviewers: palmer-dabbelt, sunshaoce, craig.topper, kito-cheng, 
jrtc27, frasercrmck, asb, luismarques.
Herald added subscribers: VincentWu, luke957, StephenFan, vkmr, jdoerfert, 
evandro, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, 
the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, shiva0217, 
niosHD, sabuasal, bollu, simoncook, johnrusso, rbar, hiraditya, arichardson, 
mcrosier.
Herald added a project: All.
reames requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, eopXD, MaskRay.
Herald added projects: clang, LLVM.

This implements the Zawrs specification as specified here: 
https://github.com/riscv/riscv-zawrs/releases/download/V1.0-rc3/Zawrs.pdf.  
Despite the 1.0 version name, this does not appear to have been ratified, so 
putting it under experimental for the moment.  I have been told that the 
current version is near final, and unlikely to change (again), but have nothing 
to cite on that.

This change adds assembly support, but does not include C language or IR 
intrinsics.  We can decide if we want them, and handle that in a separate patch.

There were two prior attempts at implementing this.

D128235  by @palmer-dabbelt implements a 
prior version of this extension.  Very annoyingly, the specification appears to 
have changed *without* a change in version number.  This patch also didn't make 
the extension experimental.

D129462  by @sunshaoce implements the current 
version, but was abandoned due to confusion with the prior.  Additionally, it's 
missing a few tests.  I took the .td file change and the valid assembly test 
from that change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133443

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/Zawrs-valid.s
  llvm/test/MC/RISCV/attribute-arch.s

Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -197,5 +197,8 @@
 .attribute arch, "rv32izca0p70"
 # CHECK: attribute  5, "rv32i2p0_zca0p70"
 
+.attribute arch, "rv32izawrs1p0"
+# CHECK: attribute  5, "rv32i2p0_zawrs1p0"
+
 .attribute arch, "rv32iztso0p1"
 # CHECK: attribute  5, "rv32i2p0_ztso0p1"
Index: llvm/test/MC/RISCV/Zawrs-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/Zawrs-valid.s
@@ -0,0 +1,18 @@
+# RUN: llvm-mc %s -triple=riscv32 -mattr=+experimental-zawrs -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+experimental-zawrs -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+experimental-zawrs < %s \
+# RUN: | llvm-objdump --mattr=+experimental-zawrs -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+experimental-zawrs < %s \
+# RUN: | llvm-objdump --mattr=+experimental-zawrs -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: wrs.nto
+# CHECK-ASM: encoding: [0x73,0x00,0xd0,0x00]
+wrs.nto
+
+# CHECK-ASM-AND-OBJ: wrs.sto
+# CHECK-ASM: encoding: [0x73,0x00,0xd0,0x01]
+wrs.sto
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -84,6 +84,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbom %s -o - | FileCheck --check-prefix=RV64ZICBOM %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicboz %s -o - | FileCheck --check-prefix=RV64ZICBOZ %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
 ; RV32M: .attribute 5, "rv32i2p0_m2p0"
@@ -170,6 +171,7 @@
 ; RV64COMBINEINTOZKS: .attribute 5, "rv64i2p0_zbkb1p0_zbkc1p0_zbkx1p0_zks1p0_zksed1p0_zksh1p0"
 ; RV64ZICBOM: .attribute 5, "rv64i2p0_zicbom1p0"
 ; RV64ZICBOZ: .attribute 5, "rv64i2p0_zicboz1p0"
+; RV64ZAWRS: .attribute 5, "rv64i2p0_zawrs1p0"
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSub

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:709
+
+let Predicates = [HasStdExtZawrs] in {
+def WRS_NTO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.nto", "">,

jrtc27 wrote:
> This doesn't really belong here, but a separate RISCVInstrInfoZawrs.td also 
> seems a little overkill... hmm
I had the same thoughts.  I'm happy to defer to reviewers' preference.  



Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:95
   bool HasStdExtZmmul = false;
+  bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;

jrtc27 wrote:
> I would say keep these sorted but this seems to be a bit of a mess...
I'm happy to sort in a separate change if you'd like.  Preferably after this 
lands.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133443

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

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D133443#3790420 , @asb wrote:

> The change in set of instructions without changing the version number is 
> concerning - do you know anyone involved in that group? It would be good to 
> feedback the difficulties this can cause for us. It's also not clear if there 
> might be changes again during ratification without changing the version 
> number, which wouldn't be ideal (though probably just about livable given 
> it's marked as experimental).

I agree that the incompatible change is highly sub-optimal.  I've asked around 
a bit on the status here, but nothing I would consider "official" or would want 
to quote someone on.  My overall take is that we probably should take this as 
experimental, but only with the understanding that it may change in 
incompatible ways.

> Let's discuss briefly in the meeting tomorrow. Implementation-wise it all 
> seems straightforward.

Definitely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133443

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

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D133443#3792627 , @asb wrote:

> I think the summary of our discussion on this was:
>
> - The versioning confusion is unfortunate - ideally there would be discussion 
> elsewhere at RVI on improving the situation (either ELF attributes to 
> indicate extensions are experimental, or making that unnecessary via never 
> using 1.0 until something is ratified)
> - But the above isn't a blocker to merging. As the extension is gated by the 
> experimental flag, even if there are last minute changes the impact on users 
> should be minimal / non-existent.

Matches my takeaway.  I'm going to rebase this and add in a doc change which 
clearly notes the release candidate bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133443

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

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 460447.
reames added a comment.

Add docs.


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

https://reviews.llvm.org/D133443

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/attribute-arch.s

Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -197,5 +197,8 @@
 .attribute arch, "rv32izca0p70"
 # CHECK: attribute  5, "rv32i2p0_zca0p70"
 
+.attribute arch, "rv32izawrs1p0"
+# CHECK: attribute  5, "rv32i2p0_zawrs1p0"
+
 .attribute arch, "rv32iztso0p1"
 # CHECK: attribute  5, "rv32i2p0_ztso0p1"
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -84,6 +84,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbom %s -o - | FileCheck --check-prefix=RV64ZICBOM %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicboz %s -o - | FileCheck --check-prefix=RV64ZICBOZ %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
 ; RV32M: .attribute 5, "rv32i2p0_m2p0"
@@ -170,6 +171,7 @@
 ; RV64COMBINEINTOZKS: .attribute 5, "rv64i2p0_zbkb1p0_zbkc1p0_zbkx1p0_zks1p0_zksed1p0_zksh1p0"
 ; RV64ZICBOM: .attribute 5, "rv64i2p0_zicbom1p0"
 ; RV64ZICBOZ: .attribute 5, "rv64i2p0_zicboz1p0"
+; RV64ZAWRS: .attribute 5, "rv64i2p0_zawrs1p0"
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -92,6 +92,7 @@
   bool HasStdExtZicboz = false;
   bool HasStdExtZicbop = false;
   bool HasStdExtZmmul = false;
+  bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
@@ -192,6 +193,7 @@
   bool hasStdExtZicbom() const { return HasStdExtZicbom; }
   bool hasStdExtZicboz() const { return HasStdExtZicboz; }
   bool hasStdExtZicbop() const { return HasStdExtZicbop; }
+  bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
   bool is64Bit() const { return HasRV64; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -705,6 +705,23 @@
   let rd = 0;
   let imm12 = 0b1100;
 }
+
+let Predicates = [HasStdExtZawrs] in {
+def WRS_NTO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.nto", "">,
+  Sched<[]> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm12 = 0b1101;
+}
+
+def WRS_STO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.sto", "">,
+  Sched<[]> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm12 = 0b00011101;
+}
+} // Predicates = [HasStdExtZawrs]
+
 } // hasSideEffects = 1, mayLoad = 0, mayStore = 0
 
 def CSRRW : CSR_ir<0b001, "csrrw">;
Index: llvm/lib/Target/RISCV/RISCV.td
===
--- llvm/lib/Target/RISCV/RISCV.td
+++ llvm/lib/Target/RISCV/RISCV.td
@@ -461,6 +461,13 @@
AssemblerPredicate<(all_of FeatureStdExtZtso),
"'Ztso' (Memory Model - Total Store Order)">;
 
+def FeatureStdExtZawrs
+: SubtargetFeature<"experimental-zawrs", "HasStdExtZawrs", "true",
+   "'Zawrs' (Wait on Reservation Set)">;
+def HasStdExtZawrs : Predicate<"Subtarget->hasStdExtZawrs()">,
+   AssemblerPredicate<(all_of FeatureStdExtZawrs),
+   "'Zawrs' (Wait on Reservation Set)">;
+
 // Feature32Bit exists to mark CPUs that support RV32 to distinquish them from
 // tuning CPU names.
 def Feature32Bit
Index: llvm/lib/Support/RISCVISAInfo.cpp
===
--- llvm/lib/Support/RISCVISAInfo.cpp
+++ llvm/lib/Support/RISCVISAInfo.cpp
@@ -114,6 +114,7 @@
 {"zbt", RISCVExtensionVersion{0, 93}},
 {"zca", RISCVExtensionVersion{0, 70}},
 {"zvfh", RISCVExtensionVersion{0, 1}},
+{"zawrs", RISCVExtensionVersion{1, 0}},
 {"ztso", RISCVExtensionVersion{0, 1}},
 };
 
Index: llvm/docs/RISCVUsage.rst
=

[PATCH] D130311: [RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I'm not fluent on strict FP, so let me summarize my understanding.  This is 
mostly so you can easily correct me if one my assumptions is wrong.

- Under strict FP, clang will emit constrained fp intrinsics instead of normal 
floating point ops.
- To my knowledge, clang will never emit an explicit vector constrained 
intrinsic.
- The vectorizers (LV, SLP) don't appear to have any handling for constrained 
FP intrinsics.  If it did, I'd expect it to have to ask about legality of the 
widened operation - the same way it does for e.g. a scatter/gather.

So, my question is: why don't we support StrictFP when targeting a vector 
enabled platform?  Don't we trivially support them by simply never using the 
vector forms?




Comment at: clang/lib/Basic/Targets/RISCV.cpp:286
+  // StrictFP support for vectors is incomplete.
+  if (ISAInfo->hasExtension("zve32x"))
+HasStrictFP = false;

asb wrote:
> There's also code in RISCVISAInfo.cpp that does `HasVector = 
> Exts.count("zve32x") != 0`. It's probably worth adding a helper 
> (`hasVInstructions`?) that encapsulates this, and use it from both places.
It's not clear to me why this condition is specific to embedded vector 
variants.  Do we have strict FP with +V?  Either you need to fix a comment 
here, or the condition.  One or the other.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130311

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

[PATCH] D130311: [RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D130311#3691146 , @craig.topper 
wrote:

> In D130311#3691029 , @reames wrote:
>
>> I'm not fluent on strict FP, so let me summarize my understanding.  This is 
>> mostly so you can easily correct me if one my assumptions is wrong.
>>
>> - Under strict FP, clang will emit constrained fp intrinsics instead of 
>> normal floating point ops.
>> - To my knowledge, clang will never emit an explicit vector constrained 
>> intrinsic.
>
> operator +, -, *, / etc. on __attribute__((__vector_size__)) types will 
> generate vector constrained intrinsics.

And probably also explicit intrinsic calls now that I'm thinking about it.

However, that doesn't really resolve my user interface concern.  If I have 
purely scalar code, just adding +v to the extension list at the command line 
doesn't change whether strict FP is supported or not.  This change would cause 
us to start reporting warnings which seems less than actionable to the user.  
It really seems like we need to be reporting warnings *when the explicit vector 
constructs are used*.

Worse than the warning bit, having strict FP scalar code stop being strict FP 
if you add +v seems... error prone.

(In case it's not clear, I can probably be convinced this is an imperfect step 
in the right general direction.  I just want to make sure I fully understand 
the implications before giving an LGTM.)




Comment at: clang/lib/Basic/Targets/RISCV.cpp:286
+  // StrictFP support for vectors is incomplete.
+  if (ISAInfo->hasExtension("zve32x"))
+HasStrictFP = false;

craig.topper wrote:
> reames wrote:
> > asb wrote:
> > > There's also code in RISCVISAInfo.cpp that does `HasVector = 
> > > Exts.count("zve32x") != 0`. It's probably worth adding a helper 
> > > (`hasVInstructions`?) that encapsulates this, and use it from both places.
> > It's not clear to me why this condition is specific to embedded vector 
> > variants.  Do we have strict FP with +V?  Either you need to fix a comment 
> > here, or the condition.  One or the other.  
> V implies Zve64d implies Zve64f implies Zve32f and Zve64x. Zve32f implies 
> Zve32x. Zve32x is the root of the vector inheritance tree.
So, I went digging.  I agree that our *implementation* treats V as implying 
Zve64d, but I can find anything in the *specification* to that effect.  The 
feature set seems like it might be identical between the two, but I don't see 
anything in the spec which requires a +V implementation to claim support for 
Zve64d.  Do you have particular wording in mind I'm missing?  

(Regardless, the fact we assume this elsewhere means this is a non-blocking 
comment for this review.  At the very least, this isn't introducing a new 
problem.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130311

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

[PATCH] D130311: [RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:286
+  // StrictFP support for vectors is incomplete.
+  if (ISAInfo->hasExtension("zve32x"))
+HasStrictFP = false;

craig.topper wrote:
> reames wrote:
> > craig.topper wrote:
> > > reames wrote:
> > > > asb wrote:
> > > > > There's also code in RISCVISAInfo.cpp that does `HasVector = 
> > > > > Exts.count("zve32x") != 0`. It's probably worth adding a helper 
> > > > > (`hasVInstructions`?) that encapsulates this, and use it from both 
> > > > > places.
> > > > It's not clear to me why this condition is specific to embedded vector 
> > > > variants.  Do we have strict FP with +V?  Either you need to fix a 
> > > > comment here, or the condition.  One or the other.  
> > > V implies Zve64d implies Zve64f implies Zve32f and Zve64x. Zve32f implies 
> > > Zve32x. Zve32x is the root of the vector inheritance tree.
> > So, I went digging.  I agree that our *implementation* treats V as implying 
> > Zve64d, but I can find anything in the *specification* to that effect.  The 
> > feature set seems like it might be identical between the two, but I don't 
> > see anything in the spec which requires a +V implementation to claim 
> > support for Zve64d.  Do you have particular wording in mind I'm missing?  
> > 
> > (Regardless, the fact we assume this elsewhere means this is a non-blocking 
> > comment for this review.  At the very least, this isn't introducing a new 
> > problem.)
> We removed the implication for a brief period but Krste and Andrew disagreed. 
> I believe this is now covered by the note at the end of 
> https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#183-v-vector-extension-for-application-processors
> 
> "As is the case with other RISC-V extensions, it is valid to include 
> overlapping extensions in the same ISA string. For example, RV64GCV and 
> RV64GCV_Zve64f are both valid and equivalent ISA strings, as is 
> RV64GCV_Zve64f_Zve32x_Zvl128b."
Er, yuck that's subtle.  Not quite sure I'd read it the way you do, but your 
read is at least easily defensible.  We can wait until someone has a concrete 
case where they aren't implied before figuring out if that case is disallowed 
per the spec.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130311

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

[PATCH] D129824: [RISCV] Set triple based on -march flag which can be deduced in more generic way

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

This was very briefly discussed at today's sync up call.  We were running short 
on time, so we didn't get a chance to talk through it, but there did seem to be 
a consensus that discussion on the interface implications was needed.  This 
should hopefully be on the agenda when we talk again in two weeks.


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

https://reviews.llvm.org/D129824

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

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Coming into this late, but I'd have preferred to see this separated into at 
least two pieces.  One for each "non-obvious" adjustment, and one final one 
which just did the replace on the renaming sites.  This differs from feedback 
from other reviewers above, so don't feel bound by this in any way.  Just 
expressing the general preference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125557

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

[PATCH] D125893: [RISCV][NFC] Change interface of RVVIntrinsic::getSuffixStr

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames 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/D125893/new/

https://reviews.llvm.org/D125893

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

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D110745#3035975 , @nikic wrote:

> Sorry, but the fact that there is still no way to opt-in to the old behavior 
> is still a blocker from my side. If we can't use `dereferenceable + nofree` 
> arguments for that purpose, then we need to provide a different way to do 
> that. Like `dereferenceable + really_nofree`. It looks like the current 
> implementation doesn't even accept the `dereferenceable + nofree + noalias` 
> case originally proposed (which is pretty bad from a design perspective, but 
> would at least work fairly well for rustc in practice). I don't think that 
> our current analysis capabilities are sufficient to land this change at this 
> time.

@nikic Do you have any specific examples of where this causes a workload to 
regress?  At this point, I really need something specific as opposed to a 
general concern.  We're at the point where perfection is very much the enemy of 
the good here.  As noted, I've already spent a lot of time trying to minimize 
impact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110745

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

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.
Herald added a subscriber: jeroen.dobbelaere.

@nikic ping on previous question.  It's been a month, and this has been LGTMed. 
 Without response, I plan to land this.

In D110745#3038848 , @xbolva00 wrote:

> This really needs to be properly benchmarked.

This has been benchmarked on every workload I care about, and shows no 
interesting regressions.   Unfortunately, those are all non-public Java 
workloads,

On the C/C++ side, I don't have a ready environment in which to run anything 
representative.  From the semantic change, I wouldn't expect C++ to show much 
difference, and besides, this is fixing a long standing fairly major 
correctness issue.  If you have particular suites you care about, please run 
them and share results.

At this point, I strongly lean towards committing and letting regressions be 
reported.  We might revert, or we might simply fix forward depending on what 
comes up


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110745

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

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

[Philip Reames via Phabricator via cfe-commits]

reames abandoned this revision.
reames added a comment.

I'm stopping work on this.  This has already exceeded the amount of work which 
is worthwhile for me, and it seems there is yet more work needed.

On @nikic's prompting, I finally went ahead and got the test-suite setup and 
tested a clang version with and without the flag thrown.  Unfortunately, the 
results were not pretty.  We have significant regressions in some of the 
memset/memcpy tests.   I did not bother to dig into why in detail, but I 
suspect the lack of context sensitivity is biting us.

Between the measured regressions on both C++ and rust, I don't think this can 
go in.

At this point, I've done everything I reasonable can to drive this to 
conclusion.  My actual motivation for this was a purely defensive effort to 
avoid regressing Java performance when this someday got fixed, and to make a 
good faith effort to justify my objections to Johannes' original patches.  That 
is complete.

Frankly, I think it's incredibly unfortunate that clang has an active 
miscompile, and no one seems motivated to fix that after *years* of it being 
there.  However, I have no commercial interest in clang, and the amount of work 
that seems to be remaining is well beyond anything I'm willing to do on a 
volunteer basis.

Let me summarize some ideas on future direction for the next poor person who 
stumbles into this rats nest.

The approach taken in this round of trying to infer scoped dereferenceability 
from existing attributes and to strengthen the inference of such to cover 
practical cases has been partially successful, but I no longer believe can be 
pushed across the finish line.  The problem here is not technical, but 
political.  We appear to have unresolved disagreements about the semantics of 
attributes, and the review process towards resolving those disagreements touch 
on many otherwise disjoint parts of the project.  I would definitely not advise 
moving further in this direction unless you greatly enjoy herding cats.

We could implement a contextual dereferenceability analysis.  This is useful to 
have no matter what, but requires extending the current must-execute logic and 
finding ways to efficiently make that information available cheaply through 
much of the pass pipeline.  I have some ideas on that, and if someone wants to 
brainstorm this, feel free to reach out.  However, I think it needs to be said 
that its unclear if a "perfect" version of this analysis is enough to recover 
the scoped facts in all cases.  This is a fairly speculative approach, and it 
might not be enough.

The approach taken in D61652  of splitting the 
dereferenceability attribute into two is a bit ugly.  The objection to this 
approach in this round was mostly driven by the observation that the 
"alloc-size" attribute had the same semantic split around whether the implied 
dereferenceability was scoped or not.  The good news is that the work done in 
this round was enough to cover performance regressions from the "alloc-size" 
version, and at this point, the only checked in code for "alloc-size" uses the 
non-globally dereferenceable semantics.  (We had to because it was actively 
miscompiling otherwise.)

Personally, if I was motivated to continue working on this, I'd probably 
resurrect D61652  and call it a day.

@nikic - Since you now have the sole remaining frontend for which dropping 
global deref is a performance regression without also being a correctness fix, 
any chance you're interested in driving this further?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110745

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

[PATCH] D114963: [funcattrs] Infer writeonly argument attribute

[Philip Reames via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG740057d185ea: [funcattrs] Infer writeonly argument attribute 
(authored by reames).
Herald added subscribers: cfe-commits, kerbowa, atanasyan, jrtc27, nhaehnle, 
jvesely.
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D114963?vs=391338&id=391439#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114963

Files:
  clang/test/CodeGen/SystemZ/systemz-inline-asm.c
  clang/test/CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_VECTOR_OPERATORS.c
  clang/test/CodeGen/arm-vfp16-arguments2.cpp
  clang/test/CodeGen/mips-vector-return.c
  clang/test/CodeGen/mips64-nontrivial-return.cpp
  clang/test/CodeGen/ms-mixed-ptr-sizes.c
  clang/test/CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
  clang/test/CodeGenOpenCL/amdgpu-call-kernel.cl
  clang/test/CodeGenOpenCL/kernels-have-spir-cc-by-default.cl
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
  llvm/test/Feature/OperandBundles/pr26510.ll
  llvm/test/Transforms/Coroutines/coro-async.ll
  llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
  llvm/test/Transforms/FunctionAttrs/nocapture.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll
  llvm/test/Transforms/FunctionAttrs/writeonly.ll

Index: llvm/test/Transforms/FunctionAttrs/writeonly.ll
===
--- llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -25,13 +25,13 @@
   ret void
 }
 
-; CHECK: define void @test_store(i8* nocapture %p)
+; CHECK: define void @test_store(i8* nocapture writeonly %p)
 define void @test_store(i8* %p) {
   store i8 0, i8* %p
   ret void
 }
 
-; CHECK: define void @test_addressing(i8* nocapture %p)
+; CHECK: define void @test_addressing(i8* nocapture writeonly %p)
 define void @test_addressing(i8* %p) {
   %gep = getelementptr i8, i8* %p, i64 8
   %bitcast = bitcast i8* %gep to i32*
Index: llvm/test/Transforms/FunctionAttrs/readattrs.ll
===
--- llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -34,7 +34,7 @@
   ret void
 }
 
-; CHECK: define void @test5(i8** nocapture %p, i8* %q)
+; CHECK: define void @test5(i8** nocapture writeonly %p, i8* writeonly %q)
 ; Missed optz'n: we could make %q readnone, but don't break test6!
 define void @test5(i8** %p, i8* %q) {
   store i8* %q, i8** %p
@@ -42,7 +42,7 @@
 }
 
 declare void @test6_1()
-; CHECK: define void @test6_2(i8** nocapture %p, i8* %q)
+; CHECK: define void @test6_2(i8** nocapture writeonly %p, i8* writeonly %q)
 ; This is not a missed optz'n.
 define void @test6_2(i8** %p, i8* %q) {
   store i8* %q, i8** %p
@@ -68,7 +68,7 @@
   ret i32* %p
 }
 
-; CHECK: define void @test8_2(i32* %p)
+; CHECK: define void @test8_2(i32* writeonly %p)
 define void @test8_2(i32* %p) {
 entry:
   %call = call i32* @test8_1(i32* %p)
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -8,7 +8,7 @@
 	ret i32* %q
 }
 
-; FNATTR: define void @c2(i32* %q)
+; FNATTR: define void @c2(i32* writeonly %q)
 ; It would also be acceptable to mark %q as readnone. Update @c3 too.
 define void @c2(i32* %q) {
 	store i32* %q, i32** @g
@@ -259,7 +259,7 @@
   ret void
 }
 
-; FNATTR: @nocaptureStrip(i8* nocapture %p)
+; FNATTR: @nocaptureStrip(i8* nocapture writeonly %p)
 define void @nocaptureStrip(i8* %p) {
 entry:
   %b = call i8* @llvm.strip.invariant.group.p0i8(i8* %p)
@@ -268,7 +268,7 @@
 }
 
 @g3 = global i8* null
-; FNATTR: define void @captureStrip(i8* %p)
+; FNATTR: define void @captureStrip(i8* writeonly %p)
 define void @captureStrip(i8* %p) {
   %b = call i8* @llvm.strip.invariant.group.p0i8(i8* %p)
   store i8* %b, i8** @g3
Index: llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
===
--- llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
+++ llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
@@ -7,7 +7,7 @@
 	ret i32* %tmp
 }
 
-; CHECK: define i32* @b(i32* %q)
+; CHECK: define i32* @b(i32* writeonly %q)
 define i32* @b(i32 *%q) {
 	%mem = alloca i32*
 	store i32* %q, i32** %mem
Index: llvm/test/Transforms/Coroutines/coro-async.ll
===
--- llvm/test/Transforms/Coroutines/coro-async.ll
+++ llvm/test/Transforms/Coroutines/coro-async.ll
@@ -256,7 +256,7 @@
   unreachable
 }
 
-; CHECK-LABEL: define swiftcc void @my_async_function2(%async.task* %task, %async.actor* %actor, i8* %async.ctxt)
+; CH

[PATCH] D115003: [funcattrs] Infer writeonly argument attribute [part 2]

[Philip Reames via Phabricator via cfe-commits]

reames created this revision.
reames added reviewers: jdoerfert, anna, aeubanks, modimo, sstefan1.
Herald added subscribers: jeroen.dobbelaere, ormris, kerbowa, bollu, hiraditya, 
sbc100, nhaehnle, jvesely, mcrosier.
reames requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added projects: clang, LLVM.

This builds on the code from D114963 , and 
extends it to handle calls both direct and indirect.  I ended up deciding to do 
this in the current code structure (before a cleanup) so that the cornercases 
were a bit more obvious.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115003

Files:
  clang/test/CodeGen/arm-vfp16-arguments.c
  clang/test/CodeGenCXX/wasm-args-returns.cpp
  clang/test/CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
  llvm/test/Other/cgscc-devirt-iteration.ll
  llvm/test/Transforms/FunctionAttrs/nocapture.ll
  llvm/test/Transforms/FunctionAttrs/norecurse.ll
  llvm/test/Transforms/FunctionAttrs/writeonly.ll

Index: llvm/test/Transforms/FunctionAttrs/writeonly.ll
===
--- llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -69,7 +69,7 @@
 
 declare void @direct2_callee(i8* %p) writeonly
 
-; CHECK: define void @direct2(i8* %p)
+; CHECK: define void @direct2(i8* writeonly %p)
 define void @direct2(i8* %p) {
   call void @direct2_callee(i8* %p)
   ret void
@@ -77,7 +77,7 @@
 
 declare void @direct3_callee(i8* writeonly %p)
 
-; CHECK: define void @direct3(i8* %p)
+; CHECK: define void @direct3(i8* writeonly %p)
 define void @direct3(i8* %p) {
   call void @direct3_callee(i8* %p)
   ret void
@@ -95,7 +95,7 @@
   ret void
 }
 
-; CHECK: define void @fptr_test3(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture %f)
 define void @fptr_test3(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p) writeonly
   ret void
Index: llvm/test/Transforms/FunctionAttrs/norecurse.ll
===
--- llvm/test/Transforms/FunctionAttrs/norecurse.ll
+++ llvm/test/Transforms/FunctionAttrs/norecurse.ll
@@ -50,7 +50,7 @@
 ; CHECK: Function Attrs
 ; CHECK-SAME: nounwind
 ; CHECK-NOT: norecurse
-; CHECK-NEXT: define void @intrinsic(i8* nocapture %dest, i8* nocapture readonly %src, i32 %len)
+; CHECK-NEXT: define void @intrinsic(i8* nocapture writeonly %dest, i8* nocapture readonly %src, i32 %len)
 define void @intrinsic(i8* %dest, i8* %src, i32 %len) {
   call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 %len, i1 false)
   ret void
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -15,7 +15,7 @@
 	ret void
 }
 
-; FNATTR: define void @c3(i32* %q)
+; FNATTR: define void @c3(i32* writeonly %q)
 define void @c3(i32* %q) {
 	call void @c2(i32* %q)
 	ret void
@@ -156,7 +156,7 @@
   ret void
 }
 
-; FNATTR: define i8* @test1_2(i8* nocapture readnone %x1_2, i8* returned %y1_2, i1 %c)
+; FNATTR: define i8* @test1_2(i8* nocapture readnone %x1_2, i8* returned writeonly %y1_2, i1 %c)
 define i8* @test1_2(i8* %x1_2, i8* %y1_2, i1 %c) {
   br i1 %c, label %t, label %f
 t:
Index: llvm/test/Other/cgscc-devirt-iteration.ll
===
--- llvm/test/Other/cgscc-devirt-iteration.ll
+++ llvm/test/Other/cgscc-devirt-iteration.ll
@@ -112,7 +112,7 @@
 ; CHECK-NOT: read
 ; CHECK-SAME: noinline
 ; BEFORE-LABEL: define void @test3(i8* %src, i8* %dest, i64 %size)
-; AFTER-LABEL: define void @test3(i8* nocapture readonly %src, i8* nocapture %dest, i64 %size)
+; AFTER-LABEL: define void @test3(i8* nocapture readonly %src, i8* nocapture writeonly %dest, i64 %size)
   %fptr = alloca i8* (i8*, i8*, i64)*
   store i8* (i8*, i8*, i64)* @memcpy, i8* (i8*, i8*, i64)** %fptr
   %f = load i8* (i8*, i8*, i64)*, i8* (i8*, i8*, i64)** %fptr
Index: llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
===
--- llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
+++ llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
@@ -49,7 +49,7 @@
   ret void
 }
 
-; CHECK: define void @test2_no(i8* nocapture %p, i8* nocapture readonly %q, i64 %n) #5 {
+; CHECK: define void @test2_no(i8* nocapture writeonly %p, i8* nocapture readonly %q, i64 %n) #5 {
 define void @test2_no(i8* %p, i8* %q, i64 %n) nounwind {
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %p, i8* %q, i64 %n, i1 false), !tbaa !2
   ret void
Index: llvm/lib/Transforms/IPO/FunctionAttrs.cpp
===
--- llvm/lib/Transforms/

[PATCH] D115021: [funcatts] Rewrite callsite operand handling in memory access inference

[Philip Reames via Phabricator via cfe-commits]

reames created this revision.
reames added reviewers: jdoerfert, anna, sstefan1, aeubanks, modimo.
Herald added subscribers: ormris, bollu, hiraditya, mcrosier.
reames requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

This is a rewrite of the existing code.  It isn't even close to NFC as it fixes 
several problems with the existing code.  Changes detailed below.

1. Argument attributes on an indirect call site are honored.  We'd previously 
only been honoring these on unknown direct calls, which is just weird.
2. Consistently treat calling a function pointer as an operation which reads 
the function pointer.  Previously, we gave it different treatment based on 
which attributes were on the callsite.  (e.g. a readonly callsite vs a 
writeonly callsite changed the intepretation of the function pointer access for 
an indirect call)
3. Honor attributes on vararg function declarations when visiting a vararg 
argument.  (Previously, we aborted the entire analysis.)

In addition to the previously described functional changes, the resulting code 
is both shorter, and easier to follow.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115021

Files:
  clang/test/CodeGen/arm-cmse-attr.c
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/test/Transforms/FunctionAttrs/nocapture.ll
  llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
  llvm/test/Transforms/FunctionAttrs/writeonly.ll

Index: llvm/test/Transforms/FunctionAttrs/writeonly.ll
===
--- llvm/test/Transforms/FunctionAttrs/writeonly.ll
+++ llvm/test/Transforms/FunctionAttrs/writeonly.ll
@@ -83,19 +83,19 @@
   ret void
 }
 
-; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test1(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p)
   ret void
 }
 
-; CHECK: define void @fptr_test2(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test2(i8* writeonly %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test2(i8* %p, void (i8*)* %f) {
   call void %f(i8* writeonly %p)
   ret void
 }
 
-; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test3(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p) writeonly
   ret void
Index: llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
===
--- llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
+++ llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll
@@ -1,14 +1,8 @@
 ; RUN: opt -function-attrs -S < %s | FileCheck %s
 ; RUN: opt -passes=function-attrs -S < %s | FileCheck %s
 
-; This checks for an iterator wraparound bug in FunctionAttrs.  The previous
-; "incorrect" behavior was inferring readonly for the %x argument in @caller.
-; Inferring readonly for %x *is* actually correct, since @va_func is marked
-; readonly, but FunctionAttrs was inferring readonly for the wrong reasons (and
-; we _need_ the readonly on @va_func to trigger the problematic code path).  It
-; is possible that in the future FunctionAttrs becomes smart enough to infer
-; readonly for %x for the right reasons, and at that point this test will have
-; to be marked invalid.
+; This checks for a previously existing iterator wraparound bug in
+; FunctionAttrs, and in the process covers corner cases with varargs.
 
 declare void @llvm.va_start(i8*)
 declare void @llvm.va_end(i8*)
@@ -24,8 +18,26 @@
 }
 
 define i32 @caller(i32* %x) {
-; CHECK-LABEL: define i32 @caller(i32* nocapture %x)
+; CHECK-LABEL: define i32 @caller(i32* nocapture readonly %x)
  entry:
   call void(i32*,...) @va_func(i32* null, i32 0, i32 0, i32 0, i32* %x)
   ret i32 42
 }
+
+define void @va_func2(i32* readonly %b, ...) {
+; CHECK-LABEL: define void @va_func2(i32* nocapture readonly %b, ...)
+ entry:
+  %valist = alloca i8
+  call void @llvm.va_start(i8* %valist)
+  call void @llvm.va_end(i8* %valist)
+  %x = call i32 @caller(i32* %b)
+  ret void
+}
+
+define i32 @caller2(i32* %x, i32* %y) {
+; CHECK-LABEL: define i32 @caller2(i32* nocapture readonly %x, i32* %y)
+ entry:
+  call void(i32*,...) @va_func2(i32* %x, i32 0, i32 0, i32 0, i32* %y)
+  ret i32 42
+}
+
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -128,7 +128,7 @@
 }
 
 
-; FNATTR: define void @nc3(void ()* nocapture %p)
+; FNATTR: define void @nc3(void ()* nocapture readonly %p)
 define void @nc3(void ()* %p) {
 	call void %p()
 	ret void
@@ -141,7 +141,7 @@
 	ret void
 }
 
-; FNATTR: define void @nc5(void (i8*)* nocapture %f, i8* nocapture %

[PATCH] D115003: [funcattrs] Infer writeonly argument attribute [part 2]

[Philip Reames via Phabricator via cfe-commits]

reames planned changes to this revision.
reames added a comment.

Has the same propagation bug fixed in 7b54de5f 
, need to 
rework.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115003

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

[PATCH] D115021: [funcatts] Rewrite callsite operand handling in memory access inference

[Philip Reames via Phabricator via cfe-commits]

reames planned changes to this revision.
reames added a comment.

Needs reworked over previous patch in stack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115021

___
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

[Philip Reames via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a8f3b113d05: [clang][RISCV] Set vscale_range attribute 
based on VLEN (authored by reames).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136106

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/test/CodeGen/riscv-vector-bits-vscale-range.c
  llvm/include/llvm/Support/RISCVISAInfo.h


Index: llvm/include/llvm/Support/RISCVISAInfo.h
===
--- llvm/include/llvm/Support/RISCVISAInfo.h
+++ llvm/include/llvm/Support/RISCVISAInfo.h
@@ -60,6 +60,7 @@
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- clang/test/CodeGen/riscv-vector-bits-vscale-range.c
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -9,11 +9,17 @@
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 
--check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 
--check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-UNBOUNDED
-// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-V
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-target-feature +zvl512b -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-ZVL
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64x -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVE64
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64f 
-target-feature +f -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-ZVE64
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64d 
-target-feature +f -target-feature +d -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-ZVE64
 
 // CHECK-LABEL: @func() #0
 // CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
 // CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
 // CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
-// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-V: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-ZVL: attributes #0 = { {{.*}} vscale_range(8,1024) {{.*}} }
+// CHECK-ZVE64: attributes #0 = { {{.*}} vscale_range(1,1024) {{.*}} }
 void func(void) {}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -252,9 +252,11 @@
 return std::pair(
 LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
 
-  if (hasFeature("v"))
-// Minimum VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
-return std::pair(2, 1024);
+  if (unsigned MinVLen = ISAInfo->getMinVLen()) {
+unsigned MaxVLen = ISAInfo->getMaxVLen();
+// RISCV::RVVBitsPerBlock is 64.
+return std::pair(MinVLen/64, MaxVLen/64);
+  }
 
   return None;
 }


Index: llvm/include/llvm/Support/RISCVISAInfo.h
===
--- llvm/include/llvm/Support/RISCVISAInfo.h
+++ llvm/include/llvm/Support/RISCVISAInfo.h
@@ -60,6 +60,7 @@
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- clang/test/CodeGen/riscv-vector-bits-vscale-range.c
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -9,11 +9,17 @@
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 --ch

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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D136106#3863202 , @reames wrote:

> In D136106#3863147 , @craig.topper 
> wrote:
>
>>> In the original review, I'd mentioned that MinVLEN was sometimes zero. 
>>> That's still true, but apparently only happens if you specify multiple 
>>> extensions in the -target_feature string. So, we can at least test "v" and 
>>> "zve64x" on their own before I go figure out exactly what's going wrong 
>>> regarding e.g. parsing "+v,+zvl512b".
>>
>> I think you need to use `-target-feature "v" -target-feature "zvl512b".
>
> So, this was part right.  The other issue is I'd been using Zvl512b, and 
> apparently this bit of parsing code is case sensitive.  I'm going to look 
> into improving the error reporting here.

Replying back to myself here as I'm not quite sure where to put this...

So, it turns out target-feature is currently quite weird.  At the *clang* 
level, we require individual target-feature options for each extensions.  Using 
a compound "+v,+zvl512b" is not recognized by the clang code (i.e, the bit this 
patch was modifying).  However, this raw string gets joined with the valid 
attributes, and set into the IR as "target-features"="+64bit,+v,+zvl512b" on 
the function.  This string is then parsed by later consumers, and thus the 
extensions listed in the compound list *are* picked up during e.g. codegen.  
So, it turns out we both do, and don't, parse these compound target-feature 
strings.

I'm leaning in the direction of having clang just support the compound 
target-feature strings, but I haven't found the right place for that just yet.


Repository:
  rG LLVM Github Monorepo

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] D116735: [RISCV] Adjust RISCV data layout by using n32:64 in layout string

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Just want to note that I have no strong opinion on this patch.  It doesn't seem 
unreasonable, and I'm comfortable with this being an empirically driven 
decision.


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] D136570: [RISCV] Add Svnapot extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

https://llvm.org/docs/RISCVUsage.html#extensions needs to updated after this 
change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136570

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

[PATCH] D136571: [RISCV] add svinval extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Please update https://llvm.org/docs/RISCVUsage.html#extensions

It seems like we'd previously accept these instruction without the extension 
being explicitly named?  If so, this is probably worth a release note to 
document the change in user behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136571

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

[PATCH] D136511: [RISCV][clang] Support RISC-V vectors in UninitializedValues.

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames 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/D136511/new/

https://reviews.llvm.org/D136511

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

[PATCH] D136571: [RISCV] add svinval extension

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/RISCVUsage.rst:56
  ``V``Supported
+ ``Svinval``  Assembly Support
  ``Zba``  Supported

This table is sorted alphabetically, please move above V.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136571

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

[PATCH] D136571: [RISCV] add svinval extension

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames 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/D136571/new/

https://reviews.llvm.org/D136571

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

[PATCH] D136817: [RISCV] Add H extension

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/RISCVUsage.rst:54
  ``F``Supported
+ ``H``Supported
  ``M``Supported

If I'm reading the code right here, we only have assembly support here.  Given 
that, shouldn't this be Assembly Support?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136817

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

[PATCH] D131708: [RISCV] Change how mtune aliases are implemented.

[Philip Reames via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/docs/ReleaseNotes.rst:197
+
+- ``sifive-7-rv32`` and ``sifive-7-rv64`` are no longer supported for `-mcpu`.
+

Can you given any guidance on what to use instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131708

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

[PATCH] D132197: [RISCV] Use Triple::isRISCV/isRISCV32/isRISCV64 helps in some places. NFC

[Philip Reames via Phabricator via cfe-commits]

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

LGTM w/ optional comment.




Comment at: clang/lib/Driver/ToolChains/Linux.cpp:741
  getTriple().getArch() == llvm::Triple::thumbeb;
-  const bool IsRISCV64 = getTriple().getArch() == llvm::Triple::riscv64;
+  const bool IsRISCV64 = getTriple().isRISCV64();
   const bool IsSystemZ = getTriple().getArch() == llvm::Triple::systemz;

I would leave this one unchanged for consistency with surrounding code.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132197

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

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D133443#3795603 , @asb wrote:

> Everything that's in this patch looks good to me - it's just missing some 
> simple round-trip tests in the style of rv32zicboz-valid.s (and perhaps an 
> -invalid.s that shows a sensible error message being produced when the 
> instructions have an argument).

You had me very confused at first as I'd written the valid tests - until I 
realized they'd been lost in the rebase.  Updated patch forthcoming.


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

https://reviews.llvm.org/D133443

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

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 461345.

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

https://reviews.llvm.org/D133443

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/attribute-arch.s

Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -197,5 +197,8 @@
 .attribute arch, "rv32izca0p70"
 # CHECK: attribute  5, "rv32i2p0_zca0p70"
 
+.attribute arch, "rv32izawrs1p0"
+# CHECK: attribute  5, "rv32i2p0_zawrs1p0"
+
 .attribute arch, "rv32iztso0p1"
 # CHECK: attribute  5, "rv32i2p0_ztso0p1"
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -84,6 +84,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbom %s -o - | FileCheck --check-prefix=RV64ZICBOM %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicboz %s -o - | FileCheck --check-prefix=RV64ZICBOZ %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
 ; RV32M: .attribute 5, "rv32i2p0_m2p0"
@@ -170,6 +171,7 @@
 ; RV64COMBINEINTOZKS: .attribute 5, "rv64i2p0_zbkb1p0_zbkc1p0_zbkx1p0_zks1p0_zksed1p0_zksh1p0"
 ; RV64ZICBOM: .attribute 5, "rv64i2p0_zicbom1p0"
 ; RV64ZICBOZ: .attribute 5, "rv64i2p0_zicboz1p0"
+; RV64ZAWRS: .attribute 5, "rv64i2p0_zawrs1p0"
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -92,6 +92,7 @@
   bool HasStdExtZicboz = false;
   bool HasStdExtZicbop = false;
   bool HasStdExtZmmul = false;
+  bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
@@ -192,6 +193,7 @@
   bool hasStdExtZicbom() const { return HasStdExtZicbom; }
   bool hasStdExtZicboz() const { return HasStdExtZicboz; }
   bool hasStdExtZicbop() const { return HasStdExtZicbop; }
+  bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
   bool is64Bit() const { return HasRV64; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -705,6 +705,23 @@
   let rd = 0;
   let imm12 = 0b1100;
 }
+
+let Predicates = [HasStdExtZawrs] in {
+def WRS_NTO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.nto", "">,
+  Sched<[]> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm12 = 0b1101;
+}
+
+def WRS_STO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.sto", "">,
+  Sched<[]> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm12 = 0b00011101;
+}
+} // Predicates = [HasStdExtZawrs]
+
 } // hasSideEffects = 1, mayLoad = 0, mayStore = 0
 
 def CSRRW : CSR_ir<0b001, "csrrw">;
Index: llvm/lib/Target/RISCV/RISCV.td
===
--- llvm/lib/Target/RISCV/RISCV.td
+++ llvm/lib/Target/RISCV/RISCV.td
@@ -461,6 +461,13 @@
AssemblerPredicate<(all_of FeatureStdExtZtso),
"'Ztso' (Memory Model - Total Store Order)">;
 
+def FeatureStdExtZawrs
+: SubtargetFeature<"experimental-zawrs", "HasStdExtZawrs", "true",
+   "'Zawrs' (Wait on Reservation Set)">;
+def HasStdExtZawrs : Predicate<"Subtarget->hasStdExtZawrs()">,
+   AssemblerPredicate<(all_of FeatureStdExtZawrs),
+   "'Zawrs' (Wait on Reservation Set)">;
+
 // Feature32Bit exists to mark CPUs that support RV32 to distinquish them from
 // tuning CPU names.
 def Feature32Bit
Index: llvm/lib/Support/RISCVISAInfo.cpp
===
--- llvm/lib/Support/RISCVISAInfo.cpp
+++ llvm/lib/Support/RISCVISAInfo.cpp
@@ -114,6 +114,7 @@
 {"zbt", RISCVExtensionVersion{0, 93}},
 {"zca", RISCVExtensionVersion{0, 70}},
 {"zvfh", RISCVExtensionVersion{0, 1}},
+{"zawrs", RISCVExtensionVersion{1, 0}},
 {"ztso", RISCVExtensionVersion{0, 1}},
 };
 
Index: llvm/docs/RISCVUsage.rst
=

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGeda2af575fdf: [RISCV][MC] Add support for experimental Zawrs 
extension (authored by reames).

Changed prior to commit:
  https://reviews.llvm.org/D133443?vs=461345&id=461608#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133443

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/Zawrs-valid.s
  llvm/test/MC/RISCV/attribute-arch.s

Index: llvm/test/MC/RISCV/attribute-arch.s
===
--- llvm/test/MC/RISCV/attribute-arch.s
+++ llvm/test/MC/RISCV/attribute-arch.s
@@ -197,5 +197,8 @@
 .attribute arch, "rv32izca0p70"
 # CHECK: attribute  5, "rv32i2p0_zca0p70"
 
+.attribute arch, "rv32izawrs1p0"
+# CHECK: attribute  5, "rv32i2p0_zawrs1p0"
+
 .attribute arch, "rv32iztso0p1"
 # CHECK: attribute  5, "rv32i2p0_ztso0p1"
Index: llvm/test/MC/RISCV/Zawrs-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/Zawrs-valid.s
@@ -0,0 +1,18 @@
+# RUN: llvm-mc %s -triple=riscv32 -mattr=+experimental-zawrs -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+experimental-zawrs -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+experimental-zawrs < %s \
+# RUN: | llvm-objdump --mattr=+experimental-zawrs -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+experimental-zawrs < %s \
+# RUN: | llvm-objdump --mattr=+experimental-zawrs -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: wrs.nto
+# CHECK-ASM: encoding: [0x73,0x00,0xd0,0x00]
+wrs.nto
+
+# CHECK-ASM-AND-OBJ: wrs.sto
+# CHECK-ASM: encoding: [0x73,0x00,0xd0,0x01]
+wrs.sto
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -84,6 +84,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbom %s -o - | FileCheck --check-prefix=RV64ZICBOM %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicboz %s -o - | FileCheck --check-prefix=RV64ZICBOZ %s
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
 ; RV32M: .attribute 5, "rv32i2p0_m2p0"
@@ -170,6 +171,7 @@
 ; RV64COMBINEINTOZKS: .attribute 5, "rv64i2p0_zbkb1p0_zbkc1p0_zbkx1p0_zks1p0_zksed1p0_zksh1p0"
 ; RV64ZICBOM: .attribute 5, "rv64i2p0_zicbom1p0"
 ; RV64ZICBOZ: .attribute 5, "rv64i2p0_zicboz1p0"
+; RV64ZAWRS: .attribute 5, "rv64i2p0_zawrs1p0"
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -92,6 +92,7 @@
   bool HasStdExtZicboz = false;
   bool HasStdExtZicbop = false;
   bool HasStdExtZmmul = false;
+  bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
@@ -192,6 +193,7 @@
   bool hasStdExtZicbom() const { return HasStdExtZicbom; }
   bool hasStdExtZicboz() const { return HasStdExtZicboz; }
   bool hasStdExtZicbop() const { return HasStdExtZicbop; }
+  bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
   bool is64Bit() const { return HasRV64; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -705,6 +705,23 @@
   let rd = 0;
   let imm12 = 0b1100;
 }
+
+let Predicates = [HasStdExtZawrs] in {
+def WRS_NTO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.nto", "">,
+  Sched<[]> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm12 = 0b1101;
+}
+
+def WRS_STO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.sto", "">,
+  Sched<[]> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D133443#3801899 , @asb wrote:

> It looks like they're still missing in this updated version of the patch?

I have no idea what's going wrong here.  I had been very careful to make sure 
the patch contained the new test file, but you're right, the revision in phab 
didn't.

Since we now had two LGTMs, and the tests had been in the original patch 
upload, I went ahead and landed.  The tests are in the committed patch; if you 
want any changes, let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133443

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

[PATCH] D133834: [RISCV] Remove support for the unratified Zbt extension.

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames added a comment.

LGTM as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133834

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

[PATCH] D133863: [RISCV] Add MC support of RISCV zcmt Extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Please add to the review description a link to the appropriate specification.  
Please update docs/RISCVUsage.rst to add Zcmt, and link to the specification.  
It's impossible to review e.g. encoding without knowing what you're 
implementing.




Comment at: llvm/lib/Target/RISCV/RISCV.td:367
+   [FeatureExtZca]>; // TODO: add Zicsr as another 
dependence
+def HasStdExtZcmt : Predicate<"Subtarget->hasStdExtZcmt() && 
!Subtarget->hasStdExtC()">,
+   AssemblerPredicate<(all_of FeatureExtZcmt, (not 
FeatureStdExtC)),

Wait, Zcmt can't be enabled if C is?  That seems odd...



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:1
+//===-- RISCVInstrInfoZc.td - RISC-V 'Zc' instructions -*- tablegen 
-*-===//
+//

We have Zca in RISCVInstrInfoC.td.  Not sure it makes sense to have Zcmt in one 
place, and Zca in another. 

Also, did Zca change between 0.7 and 0.7.5?  If so, any plans to update that 
extension?



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:10
+/// This file describes the RISC-V instructions from the 'Zc' Code-size 
+/// reduction extension, version 0.70.5.
+/// This version is still experimental as the 'Zc' extension hasn't been

It's not just one extension.  It is several sub-extensions.

Please reword as "from the Zc* family of code size reduction extensions, 
version ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133863

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

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

[Philip Reames via Phabricator via cfe-commits]

reames created this revision.
reames added reviewers: asb, frasercrmck, craig.topper.
Herald added subscribers: sunshaoce, VincentWu, ctetreau, StephenFan, vkmr, 
evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, 
jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, 
javed.absar, zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, bollu, 
simoncook, johnrusso, rbar, kristof.beyls, arichardson, mcrosier.
Herald added a project: All.
reames requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, eopXD, MaskRay.
Herald added a project: clang.

This follows the path that AArch64 SVE has taken.  Doing this via a function 
attribute set in the frontend is basically a workaround for the fact that 
several analyzes which need the information (i.e. known bits, lvi, scev) can't 
easily use TTI without significant amounts of plumbing changes.

This patch hard codes "v" numbers, and directly follows the SVE precedent as a 
result.  In a follow up, I hope to drive this from RISCVISAInfo.h/cpp instead, 
but the MinVLen number being returned from that interface seemed to always be 0 
(which is wrong), and I haven't figured out what's going wrong there.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135894

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


Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=2 -mvscale-max=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=4 -mvscale-max=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -mvscale-max=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -mvscale-max=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-UNBOUNDED
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+
+// CHECK-LABEL: @func() #0
+// CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
+// CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
+// CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
+// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+void func(void) {}
Index: clang/lib/Basic/Targets/RISCV.h
===
--- clang/lib/Basic/Targets/RISCV.h
+++ clang/lib/Basic/Targets/RISCV.h
@@ -90,6 +90,9 @@
  StringRef CPU,
  const std::vector &FeaturesVec) const override;
 
+  Optional>
+  getVScaleRange(const LangOptions &LangOpts) const override;
+
   bool hasFeature(StringRef Feature) const override;
 
   bool handleTargetFeatures(std::vector &Features,
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -246,6 +246,19 @@
   return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
+Optional>
+RISCVTargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
+  if (LangOpts.VScaleMin || LangOpts.VScaleMax)
+return std::pair(
+LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
+
+  if (hasFeature("v"))
+// Minium VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
+return std::

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D135894#3856229 , @craig.topper 
wrote:

> There's also this patch from @frasercrmck https://reviews.llvm.org/D107290

So there is; had not seen that.  Looking at it, that patch seems to roll 
several items together.  I'd strongly prefer if we worked incrementally here.  
I think we do need to merge the frontend and backend handling here, but we can 
we work one step at a time?

Steps as I currently see them:
Step 1 - get basic support for "v", to unblock practical codegen 
Step 2 - expose the necessary constants for frontend use, and rephrase in terms 
of VLEN
Step 3 - figure out why getMinVLen on RISCVISAInfo is currently always 
returning 0, fix that
Step 4 - revisit question if whether the result replaces TTI callback (I don't 
think it does, but don't want to speculate too much now.)

I'm willing to commit my time through at least step 3, and possibly beyond.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135894

___
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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.
Herald added subscribers: sunshaoce, StephenFan, arichardson.
Herald added a project: All.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107290

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

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 468239.
reames added a comment.

fix typo


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

https://reviews.llvm.org/D135894

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


Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=2 -mvscale-max=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=4 -mvscale-max=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -mvscale-max=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -mvscale-max=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-UNBOUNDED
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+
+// CHECK-LABEL: @func() #0
+// CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
+// CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
+// CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
+// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+void func(void) {}
Index: clang/lib/Basic/Targets/RISCV.h
===
--- clang/lib/Basic/Targets/RISCV.h
+++ clang/lib/Basic/Targets/RISCV.h
@@ -90,6 +90,9 @@
  StringRef CPU,
  const std::vector &FeaturesVec) const override;
 
+  Optional>
+  getVScaleRange(const LangOptions &LangOpts) const override;
+
   bool hasFeature(StringRef Feature) const override;
 
   bool handleTargetFeatures(std::vector &Features,
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -246,6 +246,19 @@
   return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
+Optional>
+RISCVTargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
+  if (LangOpts.VScaleMin || LangOpts.VScaleMax)
+return std::pair(
+LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
+
+  if (hasFeature("v"))
+// Minimum VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
+return std::pair(2, 1024);
+
+  return None;
+}
+
 /// Return true if has this feature, need to sync with handleTargetFeatures.
 bool RISCVTargetInfo::hasFeature(StringRef Feature) const {
   bool Is64Bit = getTriple().getArch() == llvm::Triple::riscv64;


Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=1 -mvscale-max=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=2 -mvscale-max=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=4 -mvscale-max=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=8 -mvscale-max=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-mi

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

[Philip Reames via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4467c781d7bb: [clang][RISCV] Set vscale_range attribute 
based on presence of "v" extension (authored by reames).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135894

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


Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=2 -mvscale-max=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=4 -mvscale-max=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -mvscale-max=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -mvscale-max=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 
--check-prefix=CHECK-NOMAX
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-UNBOUNDED
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+
+// CHECK-LABEL: @func() #0
+// CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
+// CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
+// CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
+// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+void func(void) {}
Index: clang/lib/Basic/Targets/RISCV.h
===
--- clang/lib/Basic/Targets/RISCV.h
+++ clang/lib/Basic/Targets/RISCV.h
@@ -90,6 +90,9 @@
  StringRef CPU,
  const std::vector &FeaturesVec) const override;
 
+  Optional>
+  getVScaleRange(const LangOptions &LangOpts) const override;
+
   bool hasFeature(StringRef Feature) const override;
 
   bool handleTargetFeatures(std::vector &Features,
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -246,6 +246,19 @@
   return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
+Optional>
+RISCVTargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
+  if (LangOpts.VScaleMin || LangOpts.VScaleMax)
+return std::pair(
+LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
+
+  if (hasFeature("v"))
+// Minimum VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
+return std::pair(2, 1024);
+
+  return None;
+}
+
 /// Return true if has this feature, need to sync with handleTargetFeatures.
 bool RISCVTargetInfo::hasFeature(StringRef Feature) const {
   bool Is64Bit = getTriple().getArch() == llvm::Triple::riscv64;


Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=1 -mvscale-max=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=2 -mvscale-max=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=4 -mvscale-max=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feat

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

[Philip Reames via Phabricator via cfe-commits]

reames created this revision.
reames added a reviewer: craig.topper.
Herald added subscribers: sunshaoce, VincentWu, StephenFan, vkmr, frasercrmck, 
evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, 
jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, 
zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, bollu, simoncook, 
johnrusso, rbar, asb, arichardson, mcrosier.
Herald added a project: All.
reames requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, eopXD, MaskRay.
Herald added projects: clang, LLVM.

Follow up on D135894 , restructure code to 
work in terms of minimum and maximum VLEN coming from RISCVISAInfo.cpp.

In the original review, I'd mentioned that MinVLEN was sometimes zero.  That's 
still true, but apparently only happens if you specify multiple extensions in 
the -target_feature string.  So, we can at least test "v" and "zve64x" on their 
own before I go figure out exactly what's going wrong regarding e.g. parsing 
"+v,+zvl512b".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136106

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/test/CodeGen/riscv-vector-bits-vscale-range.c
  llvm/include/llvm/Support/RISCVISAInfo.h


Index: llvm/include/llvm/Support/RISCVISAInfo.h
===
--- llvm/include/llvm/Support/RISCVISAInfo.h
+++ llvm/include/llvm/Support/RISCVISAInfo.h
@@ -60,6 +60,7 @@
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- clang/test/CodeGen/riscv-vector-bits-vscale-range.c
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -9,11 +9,13 @@
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 
--check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 
--check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-UNBOUNDED
-// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-V
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64x -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVE64
 
 // CHECK-LABEL: @func() #0
 // CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
 // CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
 // CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
-// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-V: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-ZVE64: attributes #0 = { {{.*}} vscale_range(1,1024) {{.*}} }
 void func(void) {}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -252,9 +252,11 @@
 return std::pair(
 LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
 
-  if (hasFeature("v"))
-// Minimum VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
-return std::pair(2, 1024);
+  if (unsigned MinVLen = ISAInfo->getMinVLen()) {
+unsigned MaxVLen = ISAInfo->getMaxVLen();
+// RISCV::RVVBitsPerBlock is 64.
+return std::pair(MinVLen/64, MaxVLen/64);
+  }
 
   return None;
 }


Index: llvm/include/llvm/Support/RISCVISAInfo.h
===
--- llvm/include/llvm/Support/RISCVISAInfo.h
+++ llvm/include/llvm/Support/RISCVISAInfo.h
@@ -60,6 +60,7 @@
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- clang/test/CodeGen/riscv-vector-bits-vscale-range.c
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -9,11 +9,13 @@
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=8 -S -emi

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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D136106#3863147 , @craig.topper 
wrote:

>> In the original review, I'd mentioned that MinVLEN was sometimes zero. 
>> That's still true, but apparently only happens if you specify multiple 
>> extensions in the -target_feature string. So, we can at least test "v" and 
>> "zve64x" on their own before I go figure out exactly what's going wrong 
>> regarding e.g. parsing "+v,+zvl512b".
>
> I think you need to use `-target-feature "v" -target-feature "zvl512b".

So, this was part right.  The other issue is I'd been using Zvl512b, and 
apparently this bit of parsing code is case sensitive.  I'm going to look into 
improving the error reporting here.


Repository:
  rG LLVM Github Monorepo

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] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 468344.
reames edited the summary of this revision.

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

https://reviews.llvm.org/D136106

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/test/CodeGen/riscv-vector-bits-vscale-range.c
  llvm/include/llvm/Support/RISCVISAInfo.h


Index: llvm/include/llvm/Support/RISCVISAInfo.h
===
--- llvm/include/llvm/Support/RISCVISAInfo.h
+++ llvm/include/llvm/Support/RISCVISAInfo.h
@@ -60,6 +60,7 @@
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- clang/test/CodeGen/riscv-vector-bits-vscale-range.c
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -9,11 +9,17 @@
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 
--check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 
--check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-UNBOUNDED
-// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-V
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v 
-target-feature +zvl512b -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-ZVL
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64x -S 
-emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVE64
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64f 
-target-feature +f -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-ZVE64
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64d 
-target-feature +f -target-feature +d -S -emit-llvm -o - %s | FileCheck %s 
--check-prefix=CHECK-ZVE64
 
 // CHECK-LABEL: @func() #0
 // CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
 // CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
 // CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
-// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-V: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-ZVL: attributes #0 = { {{.*}} vscale_range(8,1024) {{.*}} }
+// CHECK-ZVE64: attributes #0 = { {{.*}} vscale_range(1,1024) {{.*}} }
 void func(void) {}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -252,9 +252,11 @@
 return std::pair(
 LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
 
-  if (hasFeature("v"))
-// Minimum VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
-return std::pair(2, 1024);
+  if (unsigned MinVLen = ISAInfo->getMinVLen()) {
+unsigned MaxVLen = ISAInfo->getMaxVLen();
+// RISCV::RVVBitsPerBlock is 64.
+return std::pair(MinVLen/64, MaxVLen/64);
+  }
 
   return None;
 }


Index: llvm/include/llvm/Support/RISCVISAInfo.h
===
--- llvm/include/llvm/Support/RISCVISAInfo.h
+++ llvm/include/llvm/Support/RISCVISAInfo.h
@@ -60,6 +60,7 @@
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
Index: clang/test/CodeGen/riscv-vector-bits-vscale-range.c
===
--- clang/test/CodeGen/riscv-vector-bits-vscale-range.c
+++ clang/test/CodeGen/riscv-vector-bits-vscale-range.c
@@ -9,11 +9,17 @@
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-UNBOUNDED
-

[PATCH] D136817: [RISCV] Add H extension

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames 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/D136817/new/

https://reviews.llvm.org/D136817

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

[PATCH] D137266: [RISCV] Move RVVBitsPerBlock to TargetParser.h so we can use it in clang. NFC

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames 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/D137266/new/

https://reviews.llvm.org/D137266

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

[PATCH] D137280: [RISCV] Prevent autovectorization using vscale with Zvl32b.

[Philip Reames via Phabricator via cfe-commits]

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

LGTM

I'm fine with this, but I thought we didn't support zve32f during compilation 
at all right now?  Is this the only issue which needs fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137280

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

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames created this revision.
reames added reviewers: craig.topper, asb, frasercrmck, kito-cheng, jrtc27.
Herald added subscribers: sunshaoce, VincentWu, StephenFan, vkmr, jdoerfert, 
evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, 
jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, 
zzheng, shiva0217, niosHD, sabuasal, bollu, simoncook, johnrusso, rbar, 
hiraditya, arichardson, mcrosier.
Herald added a project: All.
reames requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, eopXD, MaskRay.
Herald added projects: clang, LLVM.

This change provides an implementation of the XVentanaCondOps vendor extension. 
 This extension is defined in version 1.0.0 of the VTx-family custom 
instructions specification 
(https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf)
 by Ventana Micro Systems.

This change is intended to be a test case for our vendor extension policy.  I 
believe this to be a case we clearly should accept, but it gives us an 
opportunity to discuss and set precedent on various policy and naming 
questions.  (I will comment on the review to highlight questions I think are 
worth discussion.)

I intent to bring this up at the next RISCV sync call, and ensure we have 
consensus.  Once this lands, I plan to use this extension to prototype 
selection lowering to conditional moves.  There's an RVI proposal in flight, 
and the expectation is that lowering to these and the new RVI instructions is 
likely to be substantially similar.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrFormats.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondPps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return 

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: clang/test/Preprocessor/riscv-target-features.c:437
+
+// RUN: %clang -target riscv64 -march=rv64ixventanacondops -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-XVENTANACONDOPS-EXT %s

Naming wise, is xventanacondops what we want this called?  The toolchain docs 
linked are a bit ambiguous on this point.  They seem to be saying that the 
extension should maybe be xvtcondops.  But that's not what the spec uses, not 
what we tend to use in discussion, and not what is being discussed for 
binutils.



Comment at: clang/test/Preprocessor/riscv-target-features.c:439
+// RUN: -o - | FileCheck --check-prefix=CHECK-XVENTANACONDOPS-EXT %s
+// CHECK-XVENTANACONDOPS-EXT: __riscv_xventanacondops 100{{$}}

This patch only includes positive tests for riscv64.  Per the current 
specification, no rv32 chips have shipped with this feature.  I chose not to 
actually reject the flags or elf settings for riscv32, but to not enable the 
assembler either.

What do we think is the right answer here?  Should we accept the riscv32 forms? 
 This would essentially be an LLVM extension.  Should we explicitly error on 
the riscv32 form?  Something else?



Comment at: llvm/docs/RISCVUsage.rst:161
+``XVentanaCondOps``
+  LLVM implements `version 1.0.0 of the VTx-family custom instructions 
specification 
`_
 by Ventana Micro Systems.  All instructions are prefixed with `vt.` as 
described in the specification, and the riscv-toolchai-convention document 
linked above.  These instructions are only available for riscv64 at this time.
+

Anything else we want in the documentation?  Remember, we're setting precedent 
here.  



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1785
+//===--===//
+// Vendor extensions
+//===--===//

How do we want to manage td files for vendor extensions?

I put them inline - which is probably not the right answer.  I'm leaning 
towards a vendor specific td file with extensions split out if complexity 
justifies it.  So, this patch would add a RISCVInstInfoXVentana.td.  

Is that the precedent we want here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137350

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

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 473001.
reames added a comment.

Fix silly typo


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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrFormats.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1780,3 +1780,24 @@
 include "RISCVInstrInfoV.td"
 include "RISCVInstrInfoZfh.td"
 include "RISCVInstrInfoZicbo.td"
+
+//===--===//
+// Vendor extensions
+//===--===//
+
+// -XVentanaCondOps
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0 in {
+
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b000, funct3, OPC_CUSTOM3, (outs GPR:$rd),
+  (ins GPR:$rs1, GPR:$rs2), opcodestr,
+  "$rd, $rs1, $rs2">{
+}
+
+def VT_MASKC : VTMaskedMove<0b110, "vt.maskc">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+
+def VT_MASKCN : VTMaskedMove<0b111, "vt.maskcn">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+}
Index: llvm/lib/Target/RISCV/RISCVInstrFormats.td
===
--- llvm/lib/Target/RISCV/RISCVInstrFormats.td
+++ llvm/lib/Target/RISCV/RISCVInstrFormats.td
@@ -145,6 +145,7 @@
 def OPC_JALR  : RISCVOpcode<"JALR",  0b1100111>;
 def OPC_JAL   : RISCVOpcode<"JAL",   0b110>;
 

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D137350#3906126 , @craig.topper 
wrote:

> Do these need their own DecoderNameSpace?

What is a decoder namespace?  Some quick grepping and googling isn't very 
informative.




Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:148
 def OPC_SYSTEM: RISCVOpcode<"SYSTEM",0b1110011>;
+def OPC_CUSTOM3   : RISCVOpcode<"CUSTOM3",   0b011>;
 

craig.topper wrote:
> Since the string here is used for .insn parsing, I think it should be 
> CUSTOM_3. I'm not sure why we don't already have all 4 added.
https://reviews.llvm.org/D137355


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

https://reviews.llvm.org/D137350

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

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 473022.
reames added a comment.

Address review comments from @craig.topper


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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrFormats.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1780,3 +1780,24 @@
 include "RISCVInstrInfoV.td"
 include "RISCVInstrInfoZfh.td"
 include "RISCVInstrInfoZicbo.td"
+
+//===--===//
+// Vendor extensions
+//===--===//
+
+// -XVentanaCondOps
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0 in {
+
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b000, funct3, OPC_CUSTOM3, (outs GPR:$rd),
+  (ins GPR:$rs1, GPR:$rs2), opcodestr,
+  "$rd, $rs1, $rs2">{
+}
+
+def VT_MASKC : VTMaskedMove<0b110, "vt.maskc">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+
+def VT_MASKCN : VTMaskedMove<0b111, "vt.maskcn">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+}
Index: llvm/lib/Target/RISCV/RISCVInstrFormats.td
===
--- llvm/lib/Target/RISCV/RISCVInstrFormats.td
+++ llvm/lib/Target/RISCV/RISCVInstrFormats.td
@@ -145,6 +145,7 @@
 def OPC_JALR  : RISCVOpcode<"JALR",  0b1100111>;
 def OPC_JAL   : RISCVOpcod

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 473272.
reames added a comment.

Add a decoder namespace.

I did one per vendor - i.e. used a Ventana table rather than a VentanaCondOps 
one.  In theory, we could have a vendor who ships incompatible extensions (in 
two different pieces of hardware), but if we hit that case, we can adjust 
later.  Or at least, I think we can?  This is new to me.


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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrFormats.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1780,3 +1780,25 @@
 include "RISCVInstrInfoV.td"
 include "RISCVInstrInfoZfh.td"
 include "RISCVInstrInfoZicbo.td"
+
+//===--===//
+// Vendor extensions
+//===--===//
+
+// -XVentanaCondOps
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0, DecoderNamespace = "Ventana"
+  in {
+
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b000, funct3, OPC_CUSTOM3, (outs GPR:$rd),
+  (ins GPR:$rs1, GPR:$rs2), opcodestr,
+  "$rd, $rs1, $rs2">{
+}
+
+def VT_MASKC : VTMaskedMove<0b110, "vt.maskc">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+
+def VT_MASKCN : VTMaskedMove<0b111, "vt.maskcn">,
+  

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 473285.
reames added a comment.

Rebase


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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfo.td
===
--- llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1780,3 +1780,25 @@
 include "RISCVInstrInfoV.td"
 include "RISCVInstrInfoZfh.td"
 include "RISCVInstrInfoZicbo.td"
+
+//===--===//
+// Vendor extensions
+//===--===//
+
+// -XVentanaCondOps
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0, DecoderNamespace = "Ventana"
+  in {
+
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b000, funct3, OPC_CUSTOM_3, (outs GPR:$rd),
+  (ins GPR:$rs1, GPR:$rs2), opcodestr,
+  "$rd, $rs1, $rs2">{
+}
+
+def VT_MASKC : VTMaskedMove<0b110, "vt.maskc">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+
+def VT_MASKCN : VTMaskedMove<0b111, "vt.maskcn">,
+   Sched<[WriteIALU, ReadIALU, ReadIALU]>;
+}
Index: llvm/lib/Target/RISCV/RISCV.td
===
--- llvm/lib/Target/RISCV/RISCV.td
+++ llvm/lib/Target/RISCV/RISCV.td
@@ -413,6 +413,21 @@
AssemblerPredicate<(all_of FeatureStdExtZawrs),

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 474620.
reames added a comment.

Address Kito's review comments


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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
===
--- /dev/null
+++ llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
@@ -0,0 +1,32 @@
+//===-- RISCVInstrInfoXVentana.td --*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file describes the vendor extensions defined by Ventana Micro Systems.
+//
+//===--===//
+
+//===--===//
+// -XVentanaCondOps
+//===--===//
+
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0, DecoderNamespace = "Ventana"
+  in {
+
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b000, funct3, OPC_CUSTOM_3, (outs GPR:$rd),
+  (ins GPR:$rs1, GPR:$rs2), opcodestr,
+  "$rd, $rs1, $rs2">{
+}
+
+def VT_MASKC : VTMaskedMove<0b110, 

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

reames updated this revision to Diff 474782.
reames added a comment.

Address comments from @craig.topper


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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -189,6 +190,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
===
--- /dev/null
+++ llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
@@ -0,0 +1,29 @@
+//===-- RISCVInstrInfoXVentana.td --*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file describes the vendor extensions defined by Ventana Micro Systems.
+//
+//===--===//
+
+//===--===//
+// XVentanaCondOps
+//===--===//
+
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0, DecoderNamespace = "Ventana" in
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b000, funct3, OPC_CUSTOM_3, (outs GPR:$rd),
+  (ins GPR:$rs1, GPR:$rs2), opcodestr,
+  "$rd, $rs1, $rs2"> {
+}
+
+def VT_MASKC : VTMaskedMove<0b110, "v

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Philip Reames via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG780c53984449: [RISCV] Implement assembler support for 
XVentanaCondOps (authored by reames).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137350

Files:
  clang/test/Preprocessor/riscv-target-features.c
  llvm/docs/RISCVUsage.rst
  llvm/lib/Support/RISCVISAInfo.cpp
  llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
  llvm/lib/Target/RISCV/RISCV.td
  llvm/lib/Target/RISCV/RISCVInstrInfo.td
  llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
  llvm/lib/Target/RISCV/RISCVSubtarget.h
  llvm/test/CodeGen/RISCV/attributes.ll
  llvm/test/MC/RISCV/XVentanaCondOps-valid.s

Index: llvm/test/MC/RISCV/XVentanaCondOps-valid.s
===
--- /dev/null
+++ llvm/test/MC/RISCV/XVentanaCondOps-valid.s
@@ -0,0 +1,22 @@
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -riscv-no-aliases -show-encoding \
+# RUN: | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+xventanacondops < %s \
+# RUN: | llvm-objdump --mattr=+xventanacondops -M no-aliases -d -r - \
+# RUN: | FileCheck --check-prefix=CHECK-ASM-AND-OBJ %s
+
+# CHECK-ASM-AND-OBJ: vt.maskc zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x60,0x00,0x00]
+vt.maskc x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskcn zero, zero, zero
+# CHECK-ASM: encoding: [0x7b,0x70,0x00,0x00]
+vt.maskcn x0, x0, x0
+
+# CHECK-ASM-AND-OBJ: vt.maskc ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x60,0x31,0x00]
+vt.maskc x1, x2, x3
+
+# CHECK-ASM-AND-OBJ: vt.maskcn ra, sp, gp
+# CHECK-ASM: encoding: [0xfb,0x70,0x31,0x00]
+vt.maskcn x1, x2, x3
+
Index: llvm/test/CodeGen/RISCV/attributes.ll
===
--- llvm/test/CodeGen/RISCV/attributes.ll
+++ llvm/test/CodeGen/RISCV/attributes.ll
@@ -76,6 +76,7 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+zicbop %s -o - | FileCheck --check-prefix=RV64ZICBOP %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svnapot %s -o - | FileCheck --check-prefix=RV64SVNAPOT %s
 ; RUN: llc -mtriple=riscv64 -mattr=+svinval %s -o - | FileCheck --check-prefix=RV64SVINVAL %s
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops %s -o - | FileCheck --check-prefix=RV64XVENTANACONDOPS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-zawrs %s -o - | FileCheck --check-prefix=RV64ZAWRS %s
 ; RUN: llc -mtriple=riscv64 -mattr=+experimental-ztso %s -o - | FileCheck --check-prefix=RV64ZTSO %s
 
@@ -157,6 +158,7 @@
 ; RV64ZICBOP: .attribute 5, "rv64i2p0_zicbop1p0"
 ; RV64SVNAPOT: .attribute 5, "rv64i2p0_svnapot1p0"
 ; RV64SVINVAL: .attribute 5, "rv64i2p0_svinval1p0"
+; RV64XVENTANACONDOPS: .attribute 5, "rv64i2p0_xventanacondops1p0"
 ; RV64ZTSO: .attribute 5, "rv64i2p0_ztso0p1"
 
 define i32 @addi(i32 %a) {
Index: llvm/lib/Target/RISCV/RISCVSubtarget.h
===
--- llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -90,6 +90,7 @@
   bool HasStdExtZmmul = false;
   bool HasStdExtZawrs = false;
   bool HasStdExtZtso = false;
+  bool HasVendorXVentanaCondOps = false;
   bool HasRV32 = false;
   bool HasRV64 = false;
   bool IsRV32E = false;
@@ -190,6 +191,7 @@
   bool hasStdExtZawrs() const { return HasStdExtZawrs; }
   bool hasStdExtZmmul() const { return HasStdExtZmmul; }
   bool hasStdExtZtso() const { return HasStdExtZtso; }
+  bool hasVendorXVentanaCondOps() const { return HasVendorXVentanaCondOps; }
   bool is64Bit() const { return HasRV64; }
   bool isRV32E() const { return IsRV32E; }
   bool enableLinkerRelax() const { return EnableLinkerRelax; }
Index: llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
===
--- /dev/null
+++ llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td
@@ -0,0 +1,29 @@
+//===-- RISCVInstrInfoXVentana.td --*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file describes the vendor extensions defined by Ventana Micro Systems.
+//
+//===--===//
+
+//===--===//
+// XVentanaCondOps
+//===--===//
+
+let Predicates = [IsRV64, HasVendorXVentanaCondOps], hasSideEffects = 0,
+  mayLoad = 0, mayStore = 0, isCodeGenOnly = 0, DecoderNamespace = "Ventana" in
+class VTMaskedMove funct3, string opcodestr>
+: RVInstR<0b

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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Generally supportive of having such an option, but going to defer to others on 
the review.  I don't work enough on clang to have an opinion on code here.




Comment at: clang/docs/ReleaseNotes.rst:844
   take architecture extensions from ``-march`` if both are given.
+- Added -rvv-vector-bits= option to give an upper bound on vector length. Valid
+  values are powers of 2 between 64 and 65536. We also accept "zvl" to use

MaskRay wrote:
> 
Correct me if I'm wrong, but doesn't this set both an upper and lower bound?  
If so, the wording in the note here needs changed.  If not, the naming should 
probably be something like rvv-vector-bits-max.  


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] D141672: [RISCV] Support vector crypto extension ISA string and assembly

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

This was brought up in today's RISCV sync call; I want to summarize the major 
points of discussion.

This patch is now behind the current most recent revision on the vector-crypto 
spec.  
(https://github.com/riscv/riscv-crypto/releases/download/v20230125/riscv-crypto-spec-vector.pdf)
  The assumption is that the patch needs to be updated to match most recent.  
If there's a reason why this assumption is wrong, please explicitly describe 
the argument.

There was confusion around the versioning scheme used in the vector-crypto 
spec.  It looks like newer version have moved to an internally consistent (and 
less than 1.0) version scheme, so I think this item is resolved.

There was a mention of another change being made to the spec in the near future 
and a desire to wait until that had happened.  This isn't required by our 
experimental extension policy.  However, in generally trying to minimize churn 
by delaying a couple days is not unreasonable.  I'd welcome comment from 
someone following spec changes as to whether we should delay here or not, and 
why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141672

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

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff destination isn't null

[Philip Reames via Phabricator via cfe-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Despite the comments above, the purpose of this patch remains unclear.

Per the draft spec, the relevant wording is:
"These instructions execute as a regular load except that they will only take a 
trap caused by a synchronous exception
on element 0. If element 0 raises an exception, vl is not modied, and the trap 
is taken. If an element > 0 raises an
exception, the corresponding trap is not taken, and the vector length vl is 
reduced to the index of the element that would
have raised an exception."

Working through the scenario in this patch with the destination being null, the 
expected result is for a trap to be generated (provided null is unmapped of 
course), and VL not to be modified.  In order for this change to make any 
difference in runtime behavior, the value passed must be null (or otherwise 
guaranteed to fault).  It seems very odd to me that we are modifying code which 
only runs after an instruction which is guaranteed to fault.  Is there an 
assumed runtime here which is e.g. restarting execution?

Presumably, the actual IR instruction returns the unmodified VL in the faulting 
first access case.  (If it doesn't, that's a bug we should fix.)

The last point, and it's a critical one, is that the outparam for new_vl does 
not have to be null if dest is.  Given that, I think the entire prior 
discussion on motivation here is off base.  Unless you can point to something 
in the intrinsic docs which says *explicitly* that the new_vl param to the 
intrinsic can be null when the dest is known to fault, I think we should 
strongly reject this patch.  Even if you can, I think we should first ask if 
that's a bug in the intrinsic spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126461

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

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff destination isn't null

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D126461#3597862 , @craig.topper 
wrote:

> `dst` in the patch description is not the pointer being loaded, it's the 
> pointer of where to store the new_vl. That is only thing being checked for 
> null in this patch.

I agree this is a possible interpretation, but it's also inconsistent with some 
of the other review discussion above.

@pcwang-thead Please consider clarifying the patch description a blocking item 
for future review or potential approval.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126461

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

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff new_vl output pointer isn't null

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

@craig.topper Much clearer, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126461

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

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff new_vl output pointer isn't null

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Ok, with the revised description, let me start anew in my response.

I don't have any particular problem with this change.  I do think it would be 
good to explicitly update the docs to indicate whether the param can be null or 
not, but given a) gcc has allowed it, and b) the performance impact should be 
minimal, I would see no concerns with just documenting the current gcc state 
and implementing it in clang.  That seems like the path of least resistance 
here and is a reasonable outcome.

I'm not worried about perf impact from the null check as idiomatic usage is 
likely to involve taking the address of a local variable which will be 
trivially non-null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126461

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

[PATCH] D157362: [RISCV] Add MC layer support for Zicfilp.

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/RISCVUsage.rst:120
  ``Zicboz``   Assembly Support
+ ``Zicfilp``  Assembly Support
  ``Zicntr``   (`See Note <#riscv-i2p1-note>`__)

This in the wrong place; this is not a ratified extension.  

You need to link to the right spec version as well.  This is particularly 
important here as this spec has gone through a ton of churn, and may still do 
so.  



Comment at: llvm/test/MC/RISCV/zicfilp-valid.s:17
+
+# CHECK-ASM-AND-OBJ: auipc zero, 22
+# CHECK-ASM: encoding: [0x17,0x60,0x01,0x00]

If the proper extension is provided to llvm-objdump, we really shouldn't be 
disassembling this as the auipc name, it should be the landing pad mnemonic.  

I think this is just the use of -m no-aliases above.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157362

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

[PATCH] D155081: Specify the developer policy around links to external resources

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

Removing this item seems very off topic for the change description, and 
certainly hasn't been discussed in the linked thread.  Please add this back in 
a separate commit.

(To be clear, no objections to the overall change, just the removal of the phab 
link text.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

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

[PATCH] D155081: Specify the developer policy around links to external resources

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

aaron.ballman wrote:
> aaron.ballman wrote:
> > reames wrote:
> > > Removing this item seems very off topic for the change description, and 
> > > certainly hasn't been discussed in the linked thread.  Please add this 
> > > back in a separate commit.
> > > 
> > > (To be clear, no objections to the overall change, just the removal of 
> > > the phab link text.)
> > Hmm, I thought this was obsoleted by the new text (it is covered by "other 
> > kinds of metadata"). That said, losing that link is definitely a 
> > regression, so thank you for pointing this out! I'll find a way to add it 
> > back in (either as a stand-alone bullet point or incorporated into the new 
> > text).
> I restored the link in 
> https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
>  as part of the new bullet; please let me know if you have additional 
> concerns.
That 90% covers it.  What's left is some minor framing.  I'd suggest separating 
that point into two.  The first should be recommended metadata (phab, issues 
link), and the second can be the additional metadata point.  Something like:

```
If the patch has been reviewed, add a link to its review page, as shown
  `here `_. If 
the patch fixes a bug in GitHub Issues, we encourage adding a reference to the 
issue being closed, as described `here 
`_.

It is also acceptable to add other metadata to the commit message to automate 
processes, including for downstream consumers. and including links to resources 
that are not available to the entire community. However, such links and/or 
metadata should not be used in place of making the commit message 
self-explanatory.  

```
All of the above is just reorganizing what you had written with some very minor 
copy editing.  I'd separately suggest adding the following sentence at the end 
of the second bullet.

Note that such non-public links are *only* allowed in commit messages, and 
should not be included in the submitted code.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

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

[PATCH] D155081: Specify the developer policy around links to external resources

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

aaron.ballman wrote:
> reames wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > reames wrote:
> > > > > Removing this item seems very off topic for the change description, 
> > > > > and certainly hasn't been discussed in the linked thread.  Please add 
> > > > > this back in a separate commit.
> > > > > 
> > > > > (To be clear, no objections to the overall change, just the removal 
> > > > > of the phab link text.)
> > > > Hmm, I thought this was obsoleted by the new text (it is covered by 
> > > > "other kinds of metadata"). That said, losing that link is definitely a 
> > > > regression, so thank you for pointing this out! I'll find a way to add 
> > > > it back in (either as a stand-alone bullet point or incorporated into 
> > > > the new text).
> > > I restored the link in 
> > > https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
> > >  as part of the new bullet; please let me know if you have additional 
> > > concerns.
> > That 90% covers it.  What's left is some minor framing.  I'd suggest 
> > separating that point into two.  The first should be recommended metadata 
> > (phab, issues link), and the second can be the additional metadata point.  
> > Something like:
> > 
> > ```
> > If the patch has been reviewed, add a link to its review page, as shown
> >   `here `_. 
> > If the patch fixes a bug in GitHub Issues, we encourage adding a reference 
> > to the issue being closed, as described `here 
> > `_.
> > 
> > It is also acceptable to add other metadata to the commit message to 
> > automate processes, including for downstream consumers. and including links 
> > to resources that are not available to the entire community. However, such 
> > links and/or metadata should not be used in place of making the commit 
> > message self-explanatory.  
> > 
> > ```
> > All of the above is just reorganizing what you had written with some very 
> > minor copy editing.  I'd separately suggest adding the following sentence 
> > at the end of the second bullet.
> > 
> > Note that such non-public links are *only* allowed in commit messages, and 
> > should not be included in the submitted code.  
> I did some minor rewording for clarity, so how about:
> ```
> * If the patch has been reviewed, add a link to its review page, as shown
>   `here `__.
>   If the patch fixes a bug in GitHub Issues, we encourage adding a reference 
> to
>   the issue being closed, as described
>   `here `__.
> 
> * It is also acceptable to add other metadata to the commit message to 
> automate
>   processes, including for downstream consumers. This metadata can include
>   links to resources that are not available to the entire community. However,
>   such links and/or metadata should not be used in place of making the commit
>   message self-explanatory.
> ```
> 
> > All of the above is just reorganizing what you had written with some very 
> > minor copy editing. I'd separately suggest adding the following sentence at 
> > the end of the second bullet.
> > 
> > Note that such non-public links are *only* allowed in commit messages, and 
> > should not be included in the submitted code.
> 
> I think this might need more wordsmithing, which is why I left out of the 
> simple reorganization. The non-public links aren't limited to commit messages 
> -- for example, they're fine to use in a phabricator review or github issue 
> comment, etc. So I don't want to be too restrictive with the wording, though 
> I agree with the intent. How about something along the lines of:
> 
> Note that such non-public links should not be included in the submitted code.
LGTM to both parts.  Wording is hard, and good catch on the second.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

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

[PATCH] D149248: [RISCV][MC] MC layer support for the experimental zacas extension

[Philip Reames via Phabricator via cfe-commits]

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

LGTM




Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:3338
+unsigned Rs2 = Inst.getOperand(2).getReg();
+if (Rd % 2 == 1) {
+  SMLoc Loc = Operands[1]->getStartLoc();

Very minor, but != 0 would read more naturally for me here.  Checking for odd, 
and then an error message about even requires one extra second of thought.

Same with the one below.


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

https://reviews.llvm.org/D149248

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

[PATCH] D151730: [RISCV] Support target attribute for function

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Can you separate the change to RISCVAsmPrinter.cpp into it's own review?  This 
looks useful for any case where we have functions in the same model with 
different function attributes.  The __attribute__((target...) syntax is one way 
to have that, but there are also others.  LTO will see this, and so may other 
frontends.

This will also help with review as the set of people who can review RISCV 
backend changes and clang frontend changes is fairly non-overlapping.




Comment at: llvm/test/CodeGen/RISCV/riscv-func-attr-target.ll:5
+; CHECK-NEXT: .optionarch,   +c, +v, +zifencei, +zve32f, +zve32x, 
+zve64d, +zve64f, +zve64x, +zvl128b, +zvl32b, +zvl64b
+define void @test1() #0 {
+; CHECK-LABEL: test1

This test would be a lot easier to follow if you pruned all the spurious 
attributes, and uses the inline attribute syntax rather than the #0 reference 
syntax.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151730

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

[PATCH] D151730: [RISCV] Support target attribute for function

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

FYI, this change appears to be a partial diff.  Locally, I need to add RISCV to 
supportsMultiVersioning() to get this to work at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151730

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

[PATCH] D151730: [RISCV] Support target attribute for function

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D151730#4491786 , @craig.topper 
wrote:

> In D151730#4491773 , @jrtc27 wrote:
>
>> Isn't multiversioning a separate thing that builds on top of per-function 
>> target attributes?
>
> That's what I thought. The supportsMultiVersioning call was in an earlier 
> version of the patch that I asked about.

Yeah, looks like I got confused.

I hadn't expected that target("default") would automatically be considered 
multi-versioning, even when there was one definition of the function.  As such, 
my test case was accidentally testing multi-versioning even when I hadn't meant 
to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151730

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

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Some very high level comments for the moment.

Until the spec is frozen, these need to be moved into the experimental 
namespace.  You should also add a mention of them under the experimental list 
in docs/RISCVUsage.  A link to the current spec version - along with an exact 
version you're implementing - in both the docs, and commit comment is also 
needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138807

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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Philip Reames via Phabricator via cfe-commits]

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

LGTM, but with two specific required follow ups.  If you're not comfortable 
committing to both, please don't land this one.




Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:93
+static cl::opt MaxInstCheckedForThrow(
+"max-inst-checked-for-throw-during-inlining", cl::Hidden,
+cl::desc("the maximum number of instructions analyzed for may throw during 
"

I'd suggest a name change here.  Maybe: "inliner-attribute-window"?



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1159
+
+  auto MayContainThrowingOrExitingCall = [&](Instruction *RVal,
+ Instruction *RInst) {

Pull this out as a static helper instead of a lambda, add an assert internally 
that the two instructions are in the same block.  

Why?  Because I'm 80% sure the state capture on the lambda isn't needed, and 
having it as a separate function forces that discipline.  



Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175
+  continue;
+// Sanity check that the cloned return instruction exists and is a return
+// instruction itself.

Ok, after staring at it a bit, I've convinced myself the code here is correct, 
just needlessly conservative.

What you're doing is:
If the callees return instruction and returned call both map to the same 
instructions once inlined, determine whether there's a possible exit between 
the inlined copy.

What you could be doing:
If the callee returns a call, check if *in the callee* there's a possible exit 
between call and return, then apply attribute to cloned call.

The key difference is when the caller directly returns the result vs uses it 
locally.  The result here is that your transform is much more narrow in 
applicability than it first appears.



Comment at: llvm/test/Transforms/Inline/ret_attr_update.ll:112
+  ret i8* %s
+}

There's a critical missing test case here:
- Callee and caller have the same attributes w/different values (i.e. deref)

And thinking through the code, I think there might be a bug here.  It's not a 
serious one, but the if the callee specifies a larger deref than the caller, it 
looks like the the smaller value is being written over the larger.

Actually, digging through the attribute code, I think I'm wrong about the bug.  
However, you should definitely write the test to confirm and document merging 
behaviour!

If it does turn out I'm correct, I'm fine with this being addressed in a follow 
up patch provided that the test is added in this one and isn't a functional 
issue.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D76140: [InlineFunction] update attributes during inlining

[Philip Reames via Phabricator via cfe-commits]

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

LGTM again, with minor change.

p.s. Sorry for missing the functional issue the first time.  All of the test 
changes should have made the issue obvious, but despite reading the LangRef 
description of signext, I somehow managed to miss the separation between ABI 
and optimization attributes.




Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1177
+Valid.addDereferenceableOrNullAttr(DerefOrNullBytes);
+  auto AddAttrWithoutValues =
+  [&](SmallVectorImpl &Kinds) -> void {

I'm not sure that pulling out the helper for two cases actually helps 
readability.  Can you drop this and just do the two cases directly please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140



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

[PATCH] D16963: Copy LibASTMatchersReference.html to gen'd docs

[Philip Reames via Phabricator via cfe-commits]

reames resigned from this revision.
reames added a comment.
Herald added subscribers: arphaman, mgorny.

Resigning from a stale review (2016).  Feel free to re-add if thread ever 
revived.


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

https://reviews.llvm.org/D16963



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I want to chime in support of jyknight's meta comments - particularly the one 
about the need to balance execution speed vs code size differently in hot vs 
cold code.  For our use case, we have a very large amount of branch dense known 
cold paths, and being able to only align fast path branches would be a 
substantial space savings.

I also see value in having the prefix padding feature factored out generically. 
 If that mechanism is truly measurably faster than multi-byte nops - which if I 
reading comments correctly, has been claimed but not documented or measured? - 
using it generically for other alignment purposes would likely be worthwhile.

I'd also like to see - probably in a separate patch - support for 
auto-detecting whether the host CPU needs this mitigation.  Both -mcpu=native 
and various JITs will end up needing this, having the code centralized in one 
place would be good.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

We uncovered another functional issue with this patch, or at least, the 
interaction of this patch and other parts of LLVM.  In our support for 
STATEPOINT, PATCHPOINT, and STACKMAP we use N-byte nop sequences for regions of 
code which might be patched out.  It's important that these regions are exactly 
N bytes as concurrent patching which doesn't replace an integral number of 
instructions is ill-defined on X86-64.  This patch causes the N-byte nop 
sequence to sometimes become (N+M) bytes which breaks the patching.  I believe 
that the XRAY support may have a similar issue.

More generally, I'm worried about the legality of arbitrarily prefixing 
instructions from unknown sources.  In the particular example we saw, we had 
something along the following:

.Ltmp0:
.p2align3, 0x90
(16 byte nop sequence)
.Ltmp3:
jmp *%rax

In addition to the patching legality issue above, padding the nop sequence does 
something else interesting in this example.  It changes the alignment of Ltmp3. 
 Before, Ltmp3 was always 8 byte aligned, after prefixes are added, it's not.  
It's not clear to me exactly what the required semantics here are, but we at 
least had been assuming the alignment of Ltmp3 was guaranteed in this case.  
(That's actually how we found the patching issue.)


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I've been digging through the code for this for the last day or so.  This is a 
new area for me, so it's possible I'm off base, but I have some concerns about 
the current design.

First, there appears to already be support for instruction bundling and 
alignment in the assembler today.  I stumbled across the .bundle_align_mode, 
.bundle_start, and .bundle_end mechanism 
(https://lists.llvm.org/pipermail/llvm-dev/2012-December/056723.html) which 
seems to *heavily* overlap with this proposal.  I suspect that the compiler 
support suggested by James and myself earlier in this thread could be 
implemented on to this existing mechanism.

Second, the new callbacks and infrastructure added appear to overlap heavily 
w/the MCCodePadding infrastructure.  (Which, admitted, appears unused and 
untested.)

Third, I have not see a justification for why complexity for instruction prefix 
padding is necessary.  All the effected CPUs support multi-byte nops, so we're 
talking about a *single micro op* difference between the nop form and prefix 
form.  Can anyone point to a performance delta due to this?  If not, I'd 
suggest we should start with the nop form, and then build the prefix form in a 
generic manner for all alignment varieties.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D70157#1771841 , @jyknight wrote:

> In D70157#1771832 , @reames wrote:
>
> > I've been digging through the code for this for the last day or so.  This 
> > is a new area for me, so it's possible I'm off base, but I have some 
> > concerns about the current design.
> >
> > First, there appears to already be support for instruction bundling and 
> > alignment in the assembler today.  I stumbled across the 
> > .bundle_align_mode, .bundle_start, and .bundle_end mechanism 
> > (https://lists.llvm.org/pipermail/llvm-dev/2012-December/056723.html) which 
> > seems to *heavily* overlap with this proposal.  I suspect that the compiler 
> > support suggested by James and myself earlier in this thread could be 
> > implemented on to this existing mechanism.
> >
> > Second, the new callbacks and infrastructure added appear to overlap 
> > heavily w/the MCCodePadding infrastructure.  (Which, admitted, appears 
> > unused and untested.)
>
>
> My conclusion after looking at all of that was actually that I plan to 
> propose removing both the MCCodePadding and all the bundle-padding 
> infrastructure, not add new stuff on top of it -- the former is unused, and I 
> believe the latter is only for Chrome's NaCL, which is deprecated, and fairly 
> close to being removed. If we need something similar in the future, we should 
> certainly look to both of those for inspiration, but I don't think we need to 
> be constrained by them.


I can definitely see removing the code padding stuff, since it's unused and 
untested.

As for the bundle mechanisms, why?  It seems like exactly what we're going to 
want here.  Regardless of the auto-detect feature, we're going to need a 
representation of a bundle which needs to be properly placed to avoid 
splitting, and the current code does that.  Why not reuse the, presumable 
reasonable well tested, existing infrastructure?   The only extra thing we seem 
to need is the ability to toggle off bundle formation for instruction types we 
don't care about.  Since we're going to need an assembler spelling of that 
regardless, it seems like the current code is a decent baseline?


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Recording something so I don't forget it when we get back to the prefix padding 
version.  The write up on the bundle align mode stuff mentions a concerning 
memory overhead for the feature.  Since the basic implementation techniques are 
similar, we need to make sure we assess the memory overhead of the prefix 
padding implementation.  See 
https://www.chromium.org/nativeclient/pnacl/aligned-bundling-support-in-llvm 
for context.  I don't believe this is likely to be an issue for the nop padding 
variant.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I just posted an alternate review (https://reviews.llvm.org/D71238) which 
attempts to carve out a minimum reviewable piece of complexity.  The hope is 
that we can review that one quickly (as there are fewer interacting concerns), 
and then rebase this one (possibly splitting further).

I had previously suggested in review comments that we should reuse the 
infrastructure from .bundle_align_mode.  When I sat down to actually implement 
that, I discovered that the code for that has a bunch of interacting 
assumptions about when fragments are constructed and used vs alignment 
boundaries.  I got a version of this working, but the complexity was worrisome. 
 I now suggest that we should take the rough approach sketched here (a separate 
fragment before the one being aligned), delete the essentially unused bundle 
mode code, and revisit a unified representation if needed for memory density at 
a later time.  (i.e. my previous suggestion wasn't a good one)


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In offline discussion, there was an agreement that we needed further 
coordination to make sure this patch moves forward quickly.  For that reason, 
there will be a call happening today at 4pm Pacific.  Interested parties are 
welcome to attend.

Zoom Meeting ID: 507-497-8898
https://azul.zoom.us/j/5074978898

Results of the meeting will be summarized and posted here.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Noting another issue we found in local testing (with an older version of this 
patch).  This interacts badly with the implicit exception mechanism in LLVM.  
For that mechanism, we end up generating assembly which looks more or less like 
this:
Ltmp:

  cmp %rsi, (%rdi)
  jcc 

And a side table which maps TLmp to another label so that a fault at Ltmp can 
be interpreted as an extremely expensive branch via signal handler.

The problem is that the auto-alignment of the fused branch causes padding to be 
introduced which separate the label and the faulting exception, breaking the 
mapping.

Essentially, this comes down to an implicit assumption that the label stays 
tightly bundled with the following instruction.

This can happen with either nop or prefix padding.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Here are the minutes from our phone call a few minutes ago.

Attendees: Andy Kaylor, Craig Topper, Annita Zhang, Tom Stellard, Chandler 
Carruth, Fedor Sergeev, Philip Reames, Yuanake Luo

Status Summary
==

Performance data has been posted to llvm-dev.  We had a side discussion about 
nop encoding, and it was mentioned these numbers were collected from runs 
targeting skylake (i.e. not generic x86).  This is similar to the result we 
(Azul) have collected and shared summaries of previously.

GNU patch has landed Friday - this mostly fixes assembler command line.

Discussion on Approach
==

Three major options debated:

  Assembler only - as in the current patch, assembler does all work, only 
command line flag
  Explicit Directive only - as proposed in my alternate patch, compiler decides 
exactly what instructions get aligned
  Region based directives - as proposed in James' last comment on review, 
directives enable and disable auto-padding in assembler

Use cases identified:

  compiler users
  important than assembler is self contained (i.e. don't have to know 
compiler options for reproduceability)
  inline assembly looks a lot like assembler users
  legacy assembler
  important that existing assembly works unmodified
  assembler users
  "try it and see" model vs selective enable vs selective disable
  likely need to support all three

Consensus was that the region based directives met use cases the best.  In 
particular, desire to be able to overrule default (for say, inline assembly or 
a JITs patchability assumptions) and then restore default.  Default assembler 
behavior remains unchanged.

Stawman syntax proposal

.align_branch_boundary disable/default
.align_branch_boundary enable N, instructions (fused, jcc, jmp, etc..)

We need to ensure a consensus on syntax is shared w/gnu.  Annita agreed to 
coordinate this.

Compiler would essentially just wrap generated assembly in directives.

Issue noticed while writing this up: proposed syntax assumes a default has been 
set, but doesn't give a way to set one.  This would seem to break the desired 
reproducibility property for compiled code.  Revision needed.

Push/Pop semantics were suggested at one point, but were thought to be 
non-idiomatic?

Other Topics


We very quickly discussed nop vs prefix performance.  There was a clear 
consensus in starting with nop only patch and evolving later as needed.

Next Steps
==

Annita will refresh current patch with two key changes.  1) Drop prefix support 
and simplify and 2) drop clang driver support for now.  Desire is to minimize 
cycle time before next iteration so that feedback on approach can be given 
while reviewers are still around.

Philip will prototype directive parsing support.  Annita and Yuo (??) to handle 
coordination on syntax.

Suggested patch split:

  (current patch) command line option to set default, nop only version 
w/cleaned up code as much as possible
  assembler directive support (draft by Philip in parallel)
  (future) compiler patch to wrap by default

Side note to Annita: For you to remove "hard code", you'll have to have a 
placeholder for the enable/disable interface.  That should probably be split 
and rebased in my patch.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D70157#1787403 , @annita.zhang 
wrote:

> In D70157#1787160 , @MaskRay wrote:
>
> >
>
>
>
>
> > There is a precedant: .pushsection/.popsection (MCStreamer::SectionStack). 
> > With .push_align_branch/.pop_align_branch, we probably don't need the 
> > 'switch-to-default' action.
> > 
> > I don't know how likely we may ever need nested states (e.g. an `.include` 
> > directive inside an .align_branch region where the included file has own 
> > idea about branch alignment), but .push/.pop does not seem to be more 
> > complex than disable/enable/default.
>
> I rethink about the directives and prefer the .push/.pop pair as @MaskRay 
> suggested. To be specified, I'd suggest to use .push_align_branch_boundary 
> and .pop_align_branch_boundary to align with MC command line options. They 
> will cowork with the command line options and overwrite the options if both 
> are existing.


I agree that we need the push/pop semantics.

> To be clarified, I described the behavior of the directives from my 
> understanding. Feel free to speak if you have difference opinion.
> 
> .push_align_branch_boundary [N,] [instruction,]*
> 
>   This directive specifies the beginning of a region which will overwrite the 
> value set by the command line or by the previous directive. It can represent 
> either an enabling or disabling directive controlled by parameter N. 
>   N indicates to align the branches within N byte boundary. The default value 
> is 32. If N is 0, it means the branch alignment is off within this region. 
>   Instruction specifies types of branches to align. The value is one or 
> multiple values from fused, jcc, jmp, call, ret and indirect. The default 
> value is fused, jcc and jmp. (may change later)

I'd remove the defaults.  Let's just be explicit about what is being 
enabled/disabled.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D70157#1788025 , @jyknight wrote:

> > .push_align_branch_boundary [N,] [instruction,]*
>
> I'd like to raise again the possibility of using a more general region 
> directive to denote "It is allowable to add prefixes/nops before instructions 
> in this region if the assembler wants to", as I'd started discussing in 
> https://reviews.llvm.org/D71238#1786885 (but let's move the discussion here).


James, I think this proposal is increasing the scope of this proposal too much. 
 It also ignores some of the use cases identified and described in the writeup 
(i.e. the scoped semantics).  I'm open to discussing such a feature more 
generally, but I'd prefer to see a more narrowly focused feature immediately.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Specifically on the revised patch, I remain confused by the need for multiple 
subtypes.  The need for fragments *between* the potentially fused instructions 
doesn't make sense to me.  What I was expecting to see was the following:
BoundaryAlign w/target=the branch fragment
.. some possibly empty sequence of fragments (i.e. the test/cmp/etc..) ...
the branch fragment
a new data fragment if the branch fragment was a DF

(i.e. a single BounaryAlign fragment which aligns a payload which is defined as 
"next fragment to target fragment inclusive".)

To be specific, I'd expect to see the following for an example fused sequence:

1. BoundaryAlign w/Target = 3
2. DataFragment containing TEST RAX, RAX
3. RelaxeableFragment containing JNE symbo

Why do we need anything between the two fragments of the fused pair?

(As a reminder, I am new to this code.  If I'm missing the obvious, please just 
point it out.)


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I've updated the draft assembler support in https://reviews.llvm.org/D71315 to 
match the proposal here.  Again, this is very much WIP and mostly just to show 
proposed syntax/usage.


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D70157#1789159 , @skan wrote:

> In D70157#1788445 , @reames wrote:
>
> > Specifically on the revised patch, I remain confused by the need for 
> > multiple subtypes.  The need for fragments *between* the potentially fused 
> > instructions doesn't make sense to me.  What I was expecting to see was the 
> > following:
> >  BoundaryAlign w/target=the branch fragment
> >  .. some possibly empty sequence of fragments (i.e. the test/cmp/etc..) ...
> >  the branch fragment
> >  a new data fragment if the branch fragment was a DF
> >
> > (i.e. a single BounaryAlign fragment which aligns a payload which is 
> > defined as "next fragment to target fragment inclusive".)
> >
> > To be specific, I'd expect to see the following for an example fused 
> > sequence:
> >
> > 1. BoundaryAlign w/Target = 3
> > 2. DataFragment containing TEST RAX, RAX
> > 3. RelaxeableFragment containing JNE symbo
> >
> >   Why do we need anything between the two fragments of the fused pair?
> >
> >   (As a reminder, I am new to this code.  If I'm missing the obvious, 
> > please just point it out.)
>
>
> JUMP is not always emiteed into `MCRelaxableFragment`, it also can be emitted 
> into `MCDataFragment` and  arithmetic ops with constant arguments of unknown 
> value (e.g. ADD,AND) can be emitted into `MCRelaxableFragment` ,  you can 
> find related code in `MCObjectStreamer::EmitInstructionImpl`, 
> `X86AsmBackend::mayNeedRelaxation`.  Let's say JCC is fused with TEST,  there 
> are four possible positions for JCC and CMP
>
> 1. JCC and CMP are in same `MCDataFragment`
> 2. JCC and CMP are in  two different `MCDataFragment`
> 3. JCC and CMP are in two different `MCRelaxableFragment`
> 4. JCC in a `MCRelaxableFragment`, CMP is in a `MCDataFragment`
>
>   and since `MCCompactEncodedInstFragment` is not applicable yet, i don't 
> what's its behaviour.
>
>   In order to compute the total size of CMP and JCC in 
> `MCAssembler::relaxBoundaryAlign`, I insert a `FusedJccSplit` to force CMP 
> and JCC in two fragments. Do you have any better idea?


I agree there are multiple cases here, see the original generic description 
instead of the specific example.  The general question is why a *range* of 
fragments can't be defined.  Computing the instruction size for the entire 
range then just requires walking from first to last fragment in the range 
summing the size of each.  If both instructions are within the same data 
fragment, then no relaxation is needed, and the size of both is simply the size 
of the data fragment.

Unless I'm missing something here?


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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames added a comment.

LGTM.  The patch isn't perfect, but this has reached the point where landing 
and iterating in tree is the fastest way to convergence.  To be clear, this 
LGTM *only* applies to the current patch contents, and the *internal only* flag 
names this introduces.  These may change before we expose this publicly.

Warts with the current patch we should iterate to address:

- Stylistic issues w.r.t. comments, naming, static vs member functions remain.  
None are show stoppers.
- Many of the tests can probably be simplified and condensed.
- The bundling scheme used is probably more complicated than needed (see 
previous suggestions).
- This patch doesn't include anyway to locally disable padding, and thus is 
*known incorrect* in some cases.  As this remains off by default, this is not a 
blocker for commit.

p.s. I spoke w/James this morning, and we came up with some revisions in 
approach he's going to suggest separately.  We did explicitly discuss the 
status of the current patch, and whether it needed further review cycles or 
could be iterated in tree.  Normally, I'd wait for him to summarize that 
conversation publicly, but given the time delay and vacation schedules 
involved, I want to avoid loosing another day cycle.




Comment at: llvm/include/llvm/MC/MCFragment.h:570
+private:
+  /// The size of the MCBoundaryAlignFragment.
+  uint64_t Size = 0;

Please add a note that the size is lazily set during relaxation, and is not 
meaningful before that.  



Comment at: llvm/test/MC/X86/align-branch-32-1a.s:34
+foo:
+  movl  %eax, %fs:0x1
+  pushl  %ebp

For testing alignment functionality, we can probably use a repl int3 pattern 
here.  It would make the tests much more concise, and shouldn't effect the 
logic being (currently) tested.  



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

https://reviews.llvm.org/D70157



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

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG14fc20ca6282: Align branches within 32-Byte boundary (NOP 
padding) (authored by reames).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70157

Files:
  llvm/include/llvm/MC/MCAsmBackend.h
  llvm/include/llvm/MC/MCAssembler.h
  llvm/include/llvm/MC/MCFragment.h
  llvm/include/llvm/MC/MCObjectStreamer.h
  llvm/lib/MC/MCAssembler.cpp
  llvm/lib/MC/MCFragment.cpp
  llvm/lib/MC/MCObjectStreamer.cpp
  llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
  llvm/test/MC/X86/align-branch-32-1a.s
  llvm/test/MC/X86/align-branch-64-1a.s
  llvm/test/MC/X86/align-branch-64-1b.s
  llvm/test/MC/X86/align-branch-64-1c.s
  llvm/test/MC/X86/align-branch-64-1d.s
  llvm/test/MC/X86/align-branch-64-2a.s
  llvm/test/MC/X86/align-branch-64-2b.s
  llvm/test/MC/X86/align-branch-64-2c.s
  llvm/test/MC/X86/align-branch-64-3a.s
  llvm/test/MC/X86/align-branch-64-4a.s
  llvm/test/MC/X86/align-branch-64-5a.s
  llvm/test/MC/X86/align-branch-64-5b.s

Index: llvm/test/MC/X86/align-branch-64-5b.s
===
--- /dev/null
+++ llvm/test/MC/X86/align-branch-64-5b.s
@@ -0,0 +1,50 @@
+# Check option --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp+indirect+call+ret can cowork with option --mc-relax-all
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp+indirect+call+ret --mc-relax-all %s | llvm-objdump -d  - > %t1
+# RUN: FileCheck --input-file=%t1 %s
+
+# CHECK:  foo:
+# CHECK-NEXT:0: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:8: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   10: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   18: c1 e9 02 shrl$2, %ecx
+# CHECK-NEXT:   1b: 89 d1movl%edx, %ecx
+# CHECK-NEXT:   1d: 90   nop
+# CHECK-NEXT:   1e: 90   nop
+# CHECK-NEXT:   1f: 90   nop
+# CHECK-NEXT:   20: 0f 85 f5 ff ff ffjne {{.*}}
+# CHECK-NEXT:   26: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   2e: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   36: f6 c2 02 testb   $2, %dl
+# CHECK-NEXT:   39: 0f 85 e7 ff ff ffjne {{.*}}
+# CHECK-NEXT:   3f: 90   nop
+# CHECK-NEXT:   40: e9 d6 ff ff ff   jmp {{.*}}
+# CHECK-NEXT:   45: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   4d: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   55: 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK-NEXT:   5d: 90   nop
+# CHECK-NEXT:   5e: 90   nop
+# CHECK-NEXT:   5f: 90   nop
+# CHECK-NEXT:   60: e8 9b ff ff ff   callq   {{.*}}
+# CHECK-NEXT:   65: e9 bc ff ff ff   jmp {{.*}}
+.text
+.p2align 4
+foo:
+  .rept 3
+  movl  %eax, %fs:0x1
+  .endr
+  shrl  $2, %ecx
+.L1:
+  movl  %edx, %ecx
+  jne   .L1
+.L2:
+  .rept 2
+  movl  %eax, %fs:0x1
+  .endr
+  testb $2, %dl
+  jne   .L2
+  jmp   .L1
+  .rept 3
+  movl  %eax, %fs:0x1
+  .endr
+  call  foo
+  jmp   .L2
Index: llvm/test/MC/X86/align-branch-64-5a.s
===
--- /dev/null
+++ llvm/test/MC/X86/align-branch-64-5a.s
@@ -0,0 +1,43 @@
+# Check no nop is inserted if no branch cross or is against the boundary
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp+indirect+call+ret  %s | llvm-objdump -d  - > %t1
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s | llvm-objdump -d  - > %t2
+# RUN: cmp %t1 %t2
+# RUN: FileCheck --input-file=%t1 %s
+
+# CHECK:  foo:
+# CHECK-COUNT-3:  : 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK:18: c1 e9 02 shrl$2, %ecx
+# CHECK-NEXT:   1b: 89 d1movl%edx, %ecx
+# CHECK-NEXT:   1d: 75 fcjne {{.*}}
+# CHECK-NEXT:   1f: 55   pushq   %rbp
+# CHECK-NEXT:   20: f6 c2 02 testb   $2, %dl
+# CHECK-NEXT:   23: 75 fajne {{.*}}
+# CHECK-COUNT-2:  : 64 89 04 25 01 00 00 00  movl%eax, %fs:1
+# CHECK:35: c1 e9 02 shrl   

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I've gone ahead and landed the patch so that we can iterate in tree.  See 
commit 14fc20ca62821b5f85582bf76a467d412248c248 
.

I've also landed a couple of follow up patches to address issues which would 
have otherwise required iteration on the review.  See commits 
c148e2e2ef86f53391be459752511684424f331b 
, 
4024d49edc1598a6f8017df541147b38bf1e2818 
, and 
8b725f0459eee468ed7f9935fba3278fcb4997b1 
.

I still see some room for further cleanup (i.e. the fragment range scheme and 
tests), but what's in is of reasonable quality.

There's a couple follow up patches which are probably called for, but I think 
we can work on these in parallel now.

1. We need to settle on assembler syntax.
2. We need a patch for the x86 MI to MC translation to mark regions unsafe to 
pad.  (Probably best to separate from the above for the moment.)
3. We can incrementally add support for prefix padding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70157



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

[PATCH] D146146: [Clang] Stop demoting ElementCount/TypeSize conversion errors to warnings.

[Philip Reames via Phabricator via cfe-commits]

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

This makes perfect sense to me.  LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146146

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

[PATCH] D141672: [RISCV] Support vector crypto extension ISA string and assembly

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

FYI, zvkb was discussed at the Architectural Review Committee this week and 
they've asked for some significant changes and expansions.  See the notes 
shared here: https://lists.riscv.org/g/tech-unprivileged/message/450

I actively do not think we need to wait on landing this patch until those 
expansions have been finalized.  I think is fine to land this in experimental 
state, and then transform zvkb into the requested form.   Having a generic 
vector bitmanip extension pulled out seems to make a lot of sense to me, and I 
like (most of) the proposed additions as they close holes we've encountered in 
codegen.

I've skimmed the review and didn't spot anything glaring, but want to leave 
approval to @craig.topper since he's already done one pass and is generally 
more familiar with the MC layer than I am.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141672

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

[PATCH] D144914: [Clang][Driver] Add -mcpu=help to clang

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Code and description appear out of sync.  (help != list)  Personally, I like 
the help naming a lot better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144914

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

[PATCH] D144914: [Clang][Driver] Add -mcpu=help to clang

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames 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/D144914/new/

https://reviews.llvm.org/D144914

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

[PATCH] D148066: [RISCV] Add Smaia and Ssaia extensions support

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

I would be fine landing this as experimental before ratification.  I see no 
real downside to doing that, and would essentially leave it the judgement of 
the patch author as to whether just waiting until ratification or doing the 
extra work for staging as experimental is worthwhile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148066

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

[PATCH] D149495: [RISCV] Add support for V extension in SiFive7

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

Let's go ahead and land this as is, we can rework the stylistic pieces once the 
linked patches land.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149495

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

[PATCH] D149495: [RISCV] Add support for V extension in SiFive7

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D149495#4373796 , @michaelmaitland 
wrote:

> In D149495#4373768 , @reames wrote:
>
>> Let's go ahead and land this as is, we can rework the stylistic pieces once 
>> the linked patches land.
>
> This has landed in 
> https://reviews.llvm.org/rG1a855819a87f426bdbd83c815fa47ca01fdf928f. I raised 
> my question regarding examples in my previous comment on this post because I 
> am starting to rework the stylistic concerns raised by @pcwang-thead. 
> However, I will wait until Wang's patches land before making any 
> modifications to style.

My apologies, I read right past the commit in the history.  Oops.  Sorry for 
the noise!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149495

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