[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 requested changes to this revision.
jrtc27 added inline comments.
This revision now requires changes to proceed.



Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:1-3
+; RUN: llc -verify-machineinstrs -o - %s -mtriple=riscv32-linux-gnu 
-mattr=+reserve-x18 | FileCheck --check-prefix=RISCV32 %s
+
+; RUN: llc -verify-machineinstrs -o - %s -mtriple=riscv64-linux-gnu 
-mattr=+reserve-x18 | FileCheck --check-prefix=RISCV64 %s

Please style these in the same way as the other RISC-V CodeGen tests, in terms 
of argument order, redirecting stdin rather than using `-o - %s`, and using 
riscv64 rather than riscv64-linux-gnu (unless needed?). Also use 
update_llc_test_checks.py rather than hand-writing this. And we generally use 
RV32I and RV64I (or other appropriate arch strings) instead of RISCV32 and 
RISCV64 prefixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414



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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Is there a reason for choosing X18? On AArch64 that's either a temporary or 
saved register depending on ABI, but determined to be the "platform register". 
Picking X18 on RISC-V because that's the same index as AArch64 seems a little 
arbitrary, but maybe it happens to make sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414



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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

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

1. Please use local variables with meaningful names for `RISCV::Xn` rather than 
inlining them everywhere and making it harder at a glance to work out what's 
going on without knowing what the ABI register names are.

2. Please make a `RISCVABI::getSCSPReg` or similar function to avoid 
hard-coding X18 in a bunch of places.

3. I'm not convinced a callee-saved register makes any more sense than anything 
else. Whatever you pick, compatibility only works one way round. If foo uses an 
SCS and calls bar that doesn't, then being callee saved helps you, but if bar 
calls foo then (a) foo will try to dereference SCSP (to store then later load), 
likely faulting, perhaps instead "just" clobbering arbitrary memory (b) if foo 
manages to return back to bar without faulting then bar would expect the 
register to have been saved, but it hasn't, an ABI violation. If you use a 
caller-saved register instead then bar can call foo just fine, but foo can't 
call bar as it'll clobber its SCSP. Reserving any register like this is a 
fundamental ABI incompatibility, so picking x18 because it's callee-saved is 
meaningless, and picking it to avoid duplicating 6 lines of code (or fewer, if 
the existing 6 lines are refactored) isn't a good reason either. It's 
ultimately arbitrary, but it should be at least be based on some kind of valid 
reasoning if such reasoning exists. X18 may or may not be a right answer.

  (One might initially think that this incompatibility is fine if you're 
building an executable with SCSP that calls other libraries without SCSP, but 
that breaks down as soon as callbacks exist, as well as more niche things like 
symbol preemption.)




Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:33-37
+  std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo();
+  if (std::none_of(CSI.begin(), CSI.end(), [](CalleeSavedInfo &CSR) {
+return CSR.getReg() == RISCV::X1;
+  }))
+return;

```
if (find(CSI, RISCV::X1) == CSI.end())
  return;
```
(using `llvm::find` as a convenient wrapper around `std::find`, ie shorthand 
for `std::find(CSI.begin(), CSI.end(), RISCV::X1)`). Though personally I'd 
prefer to see X1 come from `RI.getRARegister()` rather than be hard-coded; 
other functions in this file already hard-code it, but in our CHERI fork we 
need to replace RISCV::X1 with RISCV::C1 everywhere so have changed those. 
Having said that, CHERI renders a shadow call stack unnecessary, so I don't 
particularly care if it's broken there, personally. But I still think it's 
nicer code.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:54
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(RISCV::SW))
+  .addReg(RISCV::X1)

This is wrong for RV64.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:69-73
+  std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo();
+  if (std::none_of(CSI.begin(), CSI.end(), [](CalleeSavedInfo &CSR) {
+return CSR.getReg() == RISCV::X1;
+  }))
+return;

As above.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:94
+  .addImm(-SlotSize);
+  BuildMI(MBB, MI, DL, TII->get(RISCV::LW))
+  .addReg(RISCV::X1, RegState::Define)

Also wrong for RV64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414



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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32-unknown-elf -mattr=+reserve-x18 
-verify-machineinstrs < %s \
+; RUN: | FileCheck %s --check-prefix=RV32

As I said before, please just use `-mtriple=riscv32`. The `-unknown-elf` is 
implied, irrelevant and wastes space, so all the OS-independent CodeGen tests 
just specify the CPU.



Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:3
+; RUN: llc -mtriple=riscv32-unknown-elf -mattr=+reserve-x18 
-verify-machineinstrs < %s \
+; RUN: | FileCheck %s --check-prefix=RV32
+

Two extra spaces to indent the | is the predominant style.



Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:4
+; RUN: | FileCheck %s --check-prefix=RV32
+
+; RUN: llc -mtriple=riscv64-unknown-elf -mattr=+reserve-x18 
-verify-machineinstrs < %s \

Delete this blank line.



Comment at: llvm/test/CodeGen/RISCV/shadowcallstack.ll:12
+; RV32-NEXT:ret
+; RV32-NOT: x18
+;

pcc wrote:
> Shouldn't it be looking for `s2` since that's how `x18` is spelled in 
> assembly?
The -NOTs shouldn't even exist, this isn't how you use 
`update_llc_test_checks.py`. But yes, by default that's how it'll be printed 
unless you disable printing aliases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414



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

[PATCH] D78777: [AST] Use PrintingPolicy for format string diagnosis

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 created this revision.
jrtc27 added reviewers: rsmith, Anastasia.
Herald added subscribers: cfe-commits, arichardson.
Herald added a project: clang.

This is a small improvement for OpenCL diagnostics, but is also useful
for our CHERI fork, as our __capability qualifier is suppressed from
diagnostics when all pointers are capabilities, only being used when pointers
need to be explicitly opted-in to being capabilities.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78777

Files:
  clang/lib/AST/FormatString.cpp
  clang/test/SemaOpenCL/printf-format-strings.cl


Index: clang/test/SemaOpenCL/printf-format-strings.cl
===
--- clang/test/SemaOpenCL/printf-format-strings.cl
+++ clang/test/SemaOpenCL/printf-format-strings.cl
@@ -65,8 +65,8 @@
 
 kernel void format_v4f16(half4 arg_h, float4 arg_f, double4 arg_d)
 {
-  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type '__fp16 
__attribute__((ext_vector_type(4)))' but the argument has type 'double4' 
(vector of 4 'double' values)}}
-  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type '__fp16 
__attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector 
of 4 'float' values)}}
+  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type 'half 
__attribute__((ext_vector_type(4)))' but the argument has type 'double4' 
(vector of 4 'double' values)}}
+  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type 'half 
__attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector 
of 4 'float' values)}}
   printf("%v4hf\n", arg_h);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -539,7 +539,7 @@
 }
 
 std::string ArgType::getRepresentativeTypeName(ASTContext &C) const {
-  std::string S = getRepresentativeType(C).getAsString();
+  std::string S = getRepresentativeType(C).getAsString(C.getPrintingPolicy());
 
   std::string Alias;
   if (Name) {


Index: clang/test/SemaOpenCL/printf-format-strings.cl
===
--- clang/test/SemaOpenCL/printf-format-strings.cl
+++ clang/test/SemaOpenCL/printf-format-strings.cl
@@ -65,8 +65,8 @@
 
 kernel void format_v4f16(half4 arg_h, float4 arg_f, double4 arg_d)
 {
-  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type '__fp16 __attribute__((ext_vector_type(4)))' but the argument has type 'double4' (vector of 4 'double' values)}}
-  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type '__fp16 __attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector of 4 'float' values)}}
+  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type 'half __attribute__((ext_vector_type(4)))' but the argument has type 'double4' (vector of 4 'double' values)}}
+  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type 'half __attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector of 4 'float' values)}}
   printf("%v4hf\n", arg_h);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -539,7 +539,7 @@
 }
 
 std::string ArgType::getRepresentativeTypeName(ASTContext &C) const {
-  std::string S = getRepresentativeType(C).getAsString();
+  std::string S = getRepresentativeType(C).getAsString(C.getPrintingPolicy());
 
   std::string Alias;
   if (Name) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78777: [AST] Use PrintingPolicy for format string diagnosis

[Jessica Clarke via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5fee6936b8b2: [AST] Use PrintingPolicy for format string 
diagnosis (authored by jrtc27).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78777

Files:
  clang/lib/AST/FormatString.cpp
  clang/test/SemaOpenCL/printf-format-strings.cl


Index: clang/test/SemaOpenCL/printf-format-strings.cl
===
--- clang/test/SemaOpenCL/printf-format-strings.cl
+++ clang/test/SemaOpenCL/printf-format-strings.cl
@@ -65,8 +65,8 @@
 
 kernel void format_v4f16(half4 arg_h, float4 arg_f, double4 arg_d)
 {
-  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type '__fp16 
__attribute__((ext_vector_type(4)))' but the argument has type 'double4' 
(vector of 4 'double' values)}}
-  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type '__fp16 
__attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector 
of 4 'float' values)}}
+  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type 'half 
__attribute__((ext_vector_type(4)))' but the argument has type 'double4' 
(vector of 4 'double' values)}}
+  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type 'half 
__attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector 
of 4 'float' values)}}
   printf("%v4hf\n", arg_h);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -539,7 +539,7 @@
 }
 
 std::string ArgType::getRepresentativeTypeName(ASTContext &C) const {
-  std::string S = getRepresentativeType(C).getAsString();
+  std::string S = getRepresentativeType(C).getAsString(C.getPrintingPolicy());
 
   std::string Alias;
   if (Name) {


Index: clang/test/SemaOpenCL/printf-format-strings.cl
===
--- clang/test/SemaOpenCL/printf-format-strings.cl
+++ clang/test/SemaOpenCL/printf-format-strings.cl
@@ -65,8 +65,8 @@
 
 kernel void format_v4f16(half4 arg_h, float4 arg_f, double4 arg_d)
 {
-  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type '__fp16 __attribute__((ext_vector_type(4)))' but the argument has type 'double4' (vector of 4 'double' values)}}
-  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type '__fp16 __attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector of 4 'float' values)}}
+  printf("%v4hf\n", arg_d); // expected-warning{{format specifies type 'half __attribute__((ext_vector_type(4)))' but the argument has type 'double4' (vector of 4 'double' values)}}
+  printf("%v4hf\n", arg_f); // expected-warning{{format specifies type 'half __attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector of 4 'float' values)}}
   printf("%v4hf\n", arg_h);
 }
 
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -539,7 +539,7 @@
 }
 
 std::string ArgType::getRepresentativeTypeName(ASTContext &C) const {
-  std::string S = getRepresentativeType(C).getAsString();
+  std::string S = getRepresentativeType(C).getAsString(C.getPrintingPolicy());
 
   std::string Alias;
   if (Name) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

There is a possibly-compelling argument against using x18: RV32E only gives 
x0-x15, so would not be able to support the current implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414

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

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

If changes like this are required for all these tests this is going to be a 
complete pain for downstream forks like ours. At the very least, make whatever 
script you used to update these public, as I don't want to have to recreate it 
from scratch when merging this in. I had enough "fun" with the LLD 
mass-renaming (UpperCamel -> lowerCamel) and that was _with_ a 
supposedly-working script (it didn't quite do the right thing and I seem to 
recall there being two refactoring commits, only one of which had a script); I 
do not want a repeat of that experience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82317

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

[PATCH] D131141: [RISCV] Add MC support of RISCV Zcb Extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:169
+
+let isCompressOnly = true in {
+

This feels wrong to me, but decompression is a bit dodgy if you can have all of 
Zcb without some of the extensions that its instructions decompress to?



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:177
+  (C_MUL GPRC:$rs1, GPRC:$rs2)>;
+} //Predicates = [HasStdExtZcb, HasStdExtMOrZmmul]
+

Space after // (repeated multiple times)



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:195
+
+// zext.b
+let Predicates = [HasStdExtZcb] in{

These comments are pointless (repeated multiple times)



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:215
+def : CompressPat<(LBU  GPRC:$rd, GPRC:$rs1, uimm2_zc:$imm),
+  (C_LBU GPRC:$rd, GPRC:$rs1, uimm2_zc:$imm)>;
+

These don't line up, either pad them properly or don't bother at all



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:239
+let Predicates = [HasStdExtZcb] in {
+  def : InstAlias<"c.lbu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
+  def : InstAlias<"c.lhu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;

These aren't indented anywhere in llvm/lib/Target/RISCV



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:240
+  def : InstAlias<"c.lbu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
+  def : InstAlias<"c.lhu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
+  def : InstAlias<"c.lh $rd, (${rs1})",   (C_LW GPRC:$rd, GPRC:$rs1, 0)>;

Why do you have at least two spaces after the comma here for every line? If you 
want to make them line up then there should be at least one where there's only 
one space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131141

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

[PATCH] D132843: [RISCV] Ensure target features get passed to the LTO linker for RISC-V

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D132843#3770936 , @efriedma wrote:

>> So we need to keep ABI in somewhere and read that at LTO phase, the most 
>> ideal place is the module flags. We already did that[6], but that comes with 
>> a problem is it's too late to update datalayout when we start to read a 
>> module, because LLVM will use datalayout to infer some info like the 
>> alignment of loads[7], and that means we might re-read the whole LLVM IR 
>> again to get the IR with the right info, and that requires fixing multiple 
>> places in LLVM (see[2]. Still, I am not sure it's enough or not).
>
> I'm not sure how the issues with datalayout in particular end up being an 
> issue in practice.

Maybe for IR DataLayout upgrades, but those are rather rare.

> - clang shouldn't be writing out object files without a datalayout.
> - The code to infer alignment on load/store/etc. only exists for 
> compatibility with old bitcode/IR; current LLVM bitcode always marks up 
> alignment for all operations.
>
> Or do you mean something else when you say "datalayout"?
>
>> There is also an issue with how to store and determine the final LTO target 
>> features. For example, A object built with -march=rv64g and B object built 
>> with -march=rv64gc, so which arch should we use in the LTO code generation 
>> stage? see [5] for more discussion around this issue.
>
> On other targets, the instruction set used is encoded at a per-function 
> level.  So on x86, for example, you can have an "AVX" codepath and an "SSE" 
> codepath, and use runtime CPU detection to figure out which to use.

The IR records that too, but the "default" set of extensions gets encoded in 
the output file so loaders can know what instruction sets are _required_ (as 
opposed to optional via IFUNCs). It's not a perfect match, but it's about as 
good as you can get. With GNU ld these get merged together, though currently 
LLD just picks the one from the first file and ignores the rest (I think?); 
ideally the LLVM module linker would do similar merging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132843

___
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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 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", "">,

This doesn't really belong here, but a separate RISCVInstrInfoZawrs.td also 
seems a little overkill... hmm



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

I would say keep these sorted but this seems to be a bit of a mess...


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] D70401: [RISCV] CodeGen of RVE and ilp32e/lp64e ABIs

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

GCC only ever defines __riscv_32e




Comment at: clang/lib/Basic/Targets/RISCV.cpp:210
+if (Is64Bit)
+  Builder.defineMacro("__riscv_64e");
+else

Ugh, these don't align with the normal pattern. __riscv_e already exists in the 
spec, can we just leave __riscv_32e as deprecated for RV32E and not introduce 
the old-style __riscv_64e?



Comment at: clang/lib/Basic/Targets/RISCV.h:139
 if (Name == "ilp32" || Name == "ilp32f" || Name == "ilp32d") {
   ABI = Name;
   return true;

Does it matter we don't undo the effects of the RVE ABI here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401

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

[PATCH] D70401: [RISCV] CodeGen of RVE and ilp32e/lp64e ABIs

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D70401#4657098 , @jrtc27 wrote:

> GCC only ever defines __riscv_32e

Hm, seems the comments about __riscv_32e were from months ago, ignore them if 
they aren't correct or have become outdated...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401

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

[PATCH] D128612: RISC-V big-endian support implementation

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.h:113
+if (Triple.isLittleEndian())
+  resetDataLayout("e-m:e-p:32:32-i64:64-n32-S128");
+else

And please avoid repeating the whole data layout, just make the e/E a variable



Comment at: clang/lib/Driver/ToolChains/FreeBSD.cpp:235
+CmdArgs.push_back("-m");
+CmdArgs.push_back("elf32briscv");
+break;

-X to match LE. Or ditch these (and I can ditch riscv32...) since FreeBSD only 
supports riscv64.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1700
 {M.gccSuffix(),
  "/../../../../riscv64-unknown-elf/lib" + M.gccSuffix(),
  "/../../../../riscv32-unknown-elf/lib" + M.gccSuffix()});

Just shove a `+ Be +` in the middle of these two rather than introducing a 
whole new set and picking between them?



Comment at: llvm/cmake/config-ix.cmake:463
   set(LLVM_NATIVE_ARCH RISCV)
+elseif (LLVM_NATIVE_ARCH MATCHES "riscv32be")
+  set(LLVM_NATIVE_ARCH RISCV)

I believe these need to come before the unsuffixed versions to do anything, but 
also the unsuffixed versions already handle the suffixed versions correctly so 
this isn't needed?



Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:554
 .buildGraph();
-  } else {
-assert((*ELFObj)->getArch() == Triple::riscv32 &&
-   "Invalid triple for RISCV ELF object file");
+  } else if ((*ELFObj)->getArch() == Triple::riscv64be) {
+auto &ELFObjFile = cast>(**ELFObj);

Why switch to this order when before you've used 32, 64, 32be, 64be as the order



Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:65
+
+return "E-m:e-p:64:64-i64:64-i128:128-n64-S128";
+  }

As with Clang


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128612

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

[PATCH] D128726: [RISCV][NFC] Move static global variables into static variable in function.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

The commit message is poor, because the problem is not the use of static 
globals, it's the use of globals that have constructors. Moving them to be 
static function-scoped variables doesn't change the fact that they still have 
static duration; you change the scope of them but not their storage. 
Implementations can defer their initialisation until the first time the 
function is called, but do not have to. Clang, GCC and MSVC all do in fact 
perform lazy initialisation (with all the overhead that comes with to make it 
thread-safe and exception-safe) so maybe this is fine in practice. Is there 
precedent for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128726

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

[PATCH] D128726: [RISCV][NFC] Move static global variables into static variable in function.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Seems you're right, the C++11 standard does clearly say (9.7p4):

> Dynamic initialization of a block-scope variable with static storage duration 
> (6.6.4.1) or thread storage duration (6.6.4.2) is performed the first time 
> control passes through its declaration; such a variable is considered 
> initialized upon the completion of its initialization. If the initialization 
> exits by throwing an exception, the initialization is not complete, so it 
> will be tried again the next time control enters the declaration. If control 
> enters the declaration concurrently while the variable is being initialized, 
> the concurrent execution shall wait for completion of the initialization. If 
> control re-enters the declaration recursively while the variable is being 
> initialized, the behavior is undefined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128726

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

[PATCH] D129065: [RISCV][Driver] Add libm linking to `RISCVToolchain`

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Commit subject/body should say C++ in it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129065

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

[PATCH] D122335: [clang] Emit crash reproduction as a single tar file

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

As a developer who often deals with crashes locally this is more annoying; 
currently I can just point tools at the shell script and C file in /tmp and let 
them go to work reducing, but now I have to also extract the files


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

https://reviews.llvm.org/D122335

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

[PATCH] D122556: [RISCV] Add definitions for Xiangshan processors.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/include/llvm/Support/RISCVTargetParser.def:34
 PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64gc"})
+PROC(XIANGSHAN_YANQIHU,{"xiangshan-yanqihu"},FK_64BIT,{"rv64gc"})
+PROC(XIANGSHAN_NANHU,{"xiangshan-nanhu"},FK_64BIT,{"rv64imafdc_zba_zbb_zbc_zbs_zbkb_zbkc_zbkx_zknd_zkne_zknh_zksed_zksh"})

Formatting



Comment at: llvm/include/llvm/Support/RISCVTargetParser.def:35
+PROC(XIANGSHAN_YANQIHU,{"xiangshan-yanqihu"},FK_64BIT,{"rv64gc"})
+PROC(XIANGSHAN_NANHU,{"xiangshan-nanhu"},FK_64BIT,{"rv64imafdc_zba_zbb_zbc_zbs_zbkb_zbkc_zbkx_zknd_zkne_zknh_zksed_zksh"})
 

Why imafd rather than g?



Comment at: llvm/lib/Target/RISCV/RISCV.td:546
+
+def : ProcessorModel<"xiangshan-nanhu", NoSchedModel, [Feature64Bit,
+   FeatureStdExtM,

Isn't this still under development?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122556

___
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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/Driver/riscv-arch.c:410
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RUN: %clang --target=riscv32-unknown-elf -mcpu=sifive-s21 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s

This one maybe makes sense to allow, though gets murky once you have cores that 
support writable XLEN as then the same CPU exists for both RV32 and RV64 (like 
how you can ask for say haswell i386 and x86_64) so I'm not entirely sure...



Comment at: clang/test/Driver/riscv-arch.c:414
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RUN: %clang --target=riscv32-unknown-elf -mabi=lp64 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s

IMO this kind of thing should remain an error, you're asking for an ABI that 
doesn't exist for the requested architecture


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] D43630: [Driver] Fix search paths on x32

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 abandoned this revision.
jrtc27 added a comment.
Herald added a subscriber: pengfei.
Herald added a project: All.

Superseded by D52050 


Repository:
  rC Clang

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

https://reviews.llvm.org/D43630

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

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

It's just a standard linker set; the static linker combines all the input 
sections containing the array elements into a single output section. It's 
similar to init_array etc, just without the special meaning (i.e. you have to 
write the code to iterate over the array and do what you want).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105959

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

[PATCH] D106243: [Utils] Support class template specializations in update_cc_test_checks

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 created this revision.
jrtc27 added reviewers: arichardson, jdoerfert.
jrtc27 requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

ClassTemplateSpecializationDecl not within a ClassTemplateDecl
represents an explicit instatiation of a template and so should be
handled as if it were a normal CXXRecordDecl. Unfortunately, having an
equivalent for FunctionTemplateDecl remains a TODO in ASTDumper's
VisitFunctionTemplateDecl, with all the explicit instantiations just
being emitted inside the FunctionTemplateDecl along with all the other
specializations, meaning we can't easily support explicit function
instantiations in update_cc_test_checks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106243

Files:
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp.expected
  clang/test/utils/update_cc_test_checks/explicit-template-instantiation.test
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -33,8 +33,8 @@
 '%clangxx': ['--driver-mode=g++'],
 }
 
-def get_line2spell_and_mangled(args, clang_args):
-  ret = {}
+def get_line2func_list(args, clang_args):
+  ret = collections.defaultdict(list)
   # Use clang's JSON AST dump to get the mangled name
   json_dump_args = [args.clang] + clang_args + ['-fsyntax-only', '-o', '-']
   if '-cc1' not in json_dump_args:
@@ -55,26 +55,37 @@
 
   # Parse the clang JSON and add all children of type FunctionDecl.
   # TODO: Should we add checks for global variables being emitted?
-  def parse_clang_ast_json(node):
+  def parse_clang_ast_json(node, loc, search):
 node_kind = node['kind']
 # Recurse for the following nodes that can contain nested function decls:
 if node_kind in ('NamespaceDecl', 'LinkageSpecDecl', 'TranslationUnitDecl',
- 'CXXRecordDecl'):
+ 'CXXRecordDecl', 'ClassTemplateSpecializationDecl'):
+  # Specializations must use the loc from the specialization, not the
+  # template, and search for the class's spelling as the specialization
+  # does not mention the method names in the source.
+  if node_kind == 'ClassTemplateSpecializationDecl':
+inner_loc = node['loc']
+inner_search = node['name']
+  else:
+inner_loc = None
+inner_search = None
   if 'inner' in node:
 for inner in node['inner']:
-  parse_clang_ast_json(inner)
+  parse_clang_ast_json(inner, inner_loc, inner_search)
 # Otherwise we ignore everything except functions:
 if node_kind not in ('FunctionDecl', 'CXXMethodDecl', 'CXXConstructorDecl',
  'CXXDestructorDecl', 'CXXConversionDecl'):
   return
+if loc is None:
+  loc = node['loc']
 if node.get('isImplicit') is True and node.get('storageClass') == 'extern':
-  common.debug('Skipping builtin function:', node['name'], '@', node['loc'])
+  common.debug('Skipping builtin function:', node['name'], '@', loc)
   return
-common.debug('Found function:', node['kind'], node['name'], '@', node['loc'])
-line = node['loc'].get('line')
+common.debug('Found function:', node['kind'], node['name'], '@', loc)
+line = loc.get('line')
 # If there is no line it is probably a builtin function -> skip
 if line is None:
-  common.debug('Skipping function without line number:', node['name'], '@', node['loc'])
+  common.debug('Skipping function without line number:', node['name'], '@', loc)
   return
 
 # If there is no 'inner' object, it is a function declaration and we can
@@ -88,20 +99,23 @@
   has_body = True
   break
 if not has_body:
-  common.debug('Skipping function without body:', node['name'], '@', node['loc'])
+  common.debug('Skipping function without body:', node['name'], '@', loc)
   return
 spell = node['name']
+if search is None:
+  search = spell
 mangled = node.get('mangledName', spell)
-ret[int(line)-1] = (spell, mangled)
+ret[int(line)-1].append((spell, mangled, search))
 
   ast = json.loads(stdout)
   if ast['kind'] != 'TranslationUnitDecl':
 common.error('Clang AST dump JSON format changed?')
 sys.exit(2)
-  parse_clang_ast_json(ast)
+  parse_clang_ast_json(ast, None, None)
 
-  for line, func_name in sorted(ret.items()):
-common.debug('line {}: found function {}'.format(line+1, func_name), file=sys.stderr)
+  for line, funcs in sorted(ret.items()):
+for func in funcs:
+  common.debug('line {}: found function {}'.format(line+1, func), file=sys.stderr)
   if not ret:
 common.warn('Did not find any functions using', ' '.join(json_dump_args))
   return ret
@@ -

[PATCH] D106243: [Utils] Support class template specializations in update_cc_test_checks

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 updated this revision to Diff 359643.
jrtc27 added a comment.

Drop the --llvm-bin test; only basic-cplusplus.test does that (which happened 
to be the one I used as a reference), and that only needs to be done for one 
file as it has no relation to the input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106243

Files:
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp.expected
  clang/test/utils/update_cc_test_checks/explicit-template-instantiation.test
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -33,8 +33,8 @@
 '%clangxx': ['--driver-mode=g++'],
 }
 
-def get_line2spell_and_mangled(args, clang_args):
-  ret = {}
+def get_line2func_list(args, clang_args):
+  ret = collections.defaultdict(list)
   # Use clang's JSON AST dump to get the mangled name
   json_dump_args = [args.clang] + clang_args + ['-fsyntax-only', '-o', '-']
   if '-cc1' not in json_dump_args:
@@ -55,26 +55,37 @@
 
   # Parse the clang JSON and add all children of type FunctionDecl.
   # TODO: Should we add checks for global variables being emitted?
-  def parse_clang_ast_json(node):
+  def parse_clang_ast_json(node, loc, search):
 node_kind = node['kind']
 # Recurse for the following nodes that can contain nested function decls:
 if node_kind in ('NamespaceDecl', 'LinkageSpecDecl', 'TranslationUnitDecl',
- 'CXXRecordDecl'):
+ 'CXXRecordDecl', 'ClassTemplateSpecializationDecl'):
+  # Specializations must use the loc from the specialization, not the
+  # template, and search for the class's spelling as the specialization
+  # does not mention the method names in the source.
+  if node_kind == 'ClassTemplateSpecializationDecl':
+inner_loc = node['loc']
+inner_search = node['name']
+  else:
+inner_loc = None
+inner_search = None
   if 'inner' in node:
 for inner in node['inner']:
-  parse_clang_ast_json(inner)
+  parse_clang_ast_json(inner, inner_loc, inner_search)
 # Otherwise we ignore everything except functions:
 if node_kind not in ('FunctionDecl', 'CXXMethodDecl', 'CXXConstructorDecl',
  'CXXDestructorDecl', 'CXXConversionDecl'):
   return
+if loc is None:
+  loc = node['loc']
 if node.get('isImplicit') is True and node.get('storageClass') == 'extern':
-  common.debug('Skipping builtin function:', node['name'], '@', node['loc'])
+  common.debug('Skipping builtin function:', node['name'], '@', loc)
   return
-common.debug('Found function:', node['kind'], node['name'], '@', node['loc'])
-line = node['loc'].get('line')
+common.debug('Found function:', node['kind'], node['name'], '@', loc)
+line = loc.get('line')
 # If there is no line it is probably a builtin function -> skip
 if line is None:
-  common.debug('Skipping function without line number:', node['name'], '@', node['loc'])
+  common.debug('Skipping function without line number:', node['name'], '@', loc)
   return
 
 # If there is no 'inner' object, it is a function declaration and we can
@@ -88,20 +99,23 @@
   has_body = True
   break
 if not has_body:
-  common.debug('Skipping function without body:', node['name'], '@', node['loc'])
+  common.debug('Skipping function without body:', node['name'], '@', loc)
   return
 spell = node['name']
+if search is None:
+  search = spell
 mangled = node.get('mangledName', spell)
-ret[int(line)-1] = (spell, mangled)
+ret[int(line)-1].append((spell, mangled, search))
 
   ast = json.loads(stdout)
   if ast['kind'] != 'TranslationUnitDecl':
 common.error('Clang AST dump JSON format changed?')
 sys.exit(2)
-  parse_clang_ast_json(ast)
+  parse_clang_ast_json(ast, None, None)
 
-  for line, func_name in sorted(ret.items()):
-common.debug('line {}: found function {}'.format(line+1, func_name), file=sys.stderr)
+  for line, funcs in sorted(ret.items()):
+for func in funcs:
+  common.debug('line {}: found function {}'.format(line+1, func), file=sys.stderr)
   if not ret:
 common.warn('Did not find any functions using', ' '.join(json_dump_args))
   return ret
@@ -222,7 +236,7 @@
  comment_prefix='//', argparse_callback=infer_dependent_args):
 # Build a list of filechecked and non-filechecked RUN lines.
 run_list = []
-line2spell_and_mangled_list = collections.defaultdict(list)
+line2func_list = collections.defaultdict(list)
 
 subs = {
   '%s' : ti.path,
@@ -296,8 +310,8 @@
 
   # Invoke cla

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105516

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

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

This is not meant to be an -O0.5, this is meant to be an -Oepsilon. I don't 
want optimised code, I just want code that I can actually disassemble and 
understand without having to trawl through a mess of stack spills and loads. 
This is for debugging really basic bugs (either compiler or bad C/C++ input) 
that turn up even at -O0 and that you don't want optimisations for.

This is also so that the myriad of `%clang_cc1 -disable-O0-optnone | opt -S 
-mem2reg` seen in clang/tests can become `%clang_cc1 -falways-mem2reg` as the 
current way to write those tests is really clunky.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105516

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

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D105516#2889411 , @efriedma wrote:

> The part I'm most uncomfortable with is sticking "mem2reg" in a public, 
> documented driver option.  I don't want to promise that the mem2reg pass will 
> exist forever.  We should be focused on making sure the options we add are 
> stable, and compose effectively, not just being convenient for some specific 
> use.
>
> I'd be less concerned if it were just a -cc1 option; if it's for our internal 
> use, and we can throw it away if we come up with a better solution, this 
> seems okay.

I'd be ok with having it just be a -cc1 option (I didn't even actually add a 
driver test for the non-cc1 form...). I also thought about doing something like 
`-falways-regalloc` to not tie it to the pass name, but names like that are 
misleading since machine register allocation does still happen, just not on 
things that it doesn't know could be promoted from memory to registers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105516

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

[PATCH] D106347: [PoC][RISCV] Encode arch information in a new module flag meatadata 'riscv-isa-bits'.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Why can't we just save target-features itself as a module flag instead of 
inventing yet another equivalent encoding?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



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

Ignoring whether the change is actually correct, this should be capitalised as 
XLen32EEWList, but really this should actually be RV32 not XLen32 as that's not 
a term we use.



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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106518

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/Driver/riscv-abi.c:68
 
-// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64d 
-mabi=lp64d 2>&1 \
+// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64ifd 
-mabi=lp64d 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64D %s

khchen wrote:
> Why do we need to modify here?
rv64d is not a valid arch string. rv64id is though; would that work here? These 
tests are just currently testing behaviour that _should_ have been an error... 
(see also rv64 f above)



Comment at: clang/test/Driver/riscv-arch.c:198-201
-// RUN: %clang -target riscv32-unknown-elf -march=rv32e -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32E %s
-// RV32E: error: invalid arch name 'rv32e',
-// RV32E: standard user-level extension 'e'

Hm, given we don't currently support RV32E properly this should probably be an 
error still, but could be a "nicer" error than a generic "invalid arch name" 
(which is technically wrong)



Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:45
+
+  // Parse RISCV ISA info from arch string.
+  Error parse(StringRef Arch, bool EnableExperimentalExtension,

These comments aren't helpful. If you want to write full doxygen then you can, 
but a one-line comment that won't appear in documentation and is obvious from 
the function name and signature is just clutter.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:24
+namespace {
+/// Represents the major and version number components of a RISC-V extension
+struct RISCVExtensionVersion {

This seems unnecessary



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:32
+  const char *Name;
+  /// Supported version.
+  RISCVExtensionVersion Version;

Ditto for these



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+

craig.topper wrote:
> Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
I can't help but feel this is really an array of chars, not a string. We don't 
even need the trailing NUL, though double quote syntax is simpler than curly 
braces and a bunch of single-quote chars just to save a byte.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:43
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+{"i", RISCVExtensionVersion{2, 0}, /* Experimental */ false},
+{"e", RISCVExtensionVersion{1, 9}, /* Experimental */ false},

but that's the default so I'd omit it for anything other than the cases where 
it's true



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:71-72
+
+// Remove 'experimental-' if it's prefix of string,
+// return true if did the strip.
+static bool stripExperimentalPrefix(StringRef &Ext) {

Comment isn't useful, it's a trivial wrapper



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:77
+
+RISCVISAInfo::RISCVISAInfo() : XLen(0), FLen(0) {}
+

Can this not go in the header as a trivial constructor?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:112
+
+// Helper function for filtering SupportedExtensionInfos by name.
+static auto filterSupportedExtensionInfosByName(StringRef ExtName) {

Obvious comment



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:135
+  if (CheckExperimental)
+IsExperimental = stripExperimentalPrefix(Ext);
+

*Extensions* don't have an experimental- prefix, *internal target features* do, 
so something strange is going on here. This feels like we're confusing several 
things:
1. Whether or not Ext is a feature or an extension
2. Whether or not Ext is experimental
3. Whether we want to look at experimental extensions
Some of those are somewhat necessarily interwoven, but the naming does not 
currently accurately reflect what these things mean, and I would argue we 
should be very explicit and keep features and extensions separate, never using 
the same thing to represent both.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:193
+// order, but after all known standard extensions.
+Rank = AllStdExts.size() + (Ext - 'a');
+  else

This is wrong as it doesn't include the +2 to account for I and E, though does 
happen to work because A and B are both known single-letter standard extensions 
so you can't actually get it to overlap (but were AllStdExts to omit one of 
them you would be able to).

IMO though it would be cleaner to use negative numbers for I and E, as they're 
special, and that means you can just start the counting at 0 for all the 
others, not needing the +2 anywhere.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:235-236
+
+// Compare function for extension.
+// Only compare the extension name

[PATCH] D106347: [PoC][RISCV] Encode arch information in a new module flag meatadata 'riscv-isa-features'.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:835
+llvm::RISCVISAInfo::filterISAStrings(Features);
+std::vector Ops;
+if (Features.empty()) {

Why is this building a list? Just use a string so it's in the same format as 
the function attributes? We already have parsers for that. Yes, it means you do 
a small amount of redundant work but having a single format in the IR for 
representing the same thing in every place is better than having multiple 
different ways of representing the same thing.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:843
+}
+getModule().addModuleFlag(llvm::Module::AppendUnique, "riscv-isa-features",
+  llvm::MDNode::get(Ctx, Ops));

Just like we call it target-abi not riscv-abi, this should just be called 
target-features.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:132-142
+void RISCVISAInfo::filterISAStrings(std::vector &Features) {
+  erase_if(Features, [](std::string &Feature) {
+StringRef ExtName(Feature);
+assert(ExtName.size() > 1 && (ExtName[0] == '+' || ExtName[0] == '-'));
+ExtName = ExtName.drop_front(1); // Drop '+'
+stripExperimentalPrefix(ExtName);
+if (filterSupportedExtensionInfosByName(ExtName).empty())

Why do we need to do any filtering? Just emit the whole thing.



Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:53-60
+  emitTargetAttributes(ISAInfo);
+}
 
+void RISCVTargetStreamer::emitTargetAttributes(const RISCVISAInfo &ISAInfo) {
+  if (ISAInfo.hasExtension("e"))
+emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4);
+  else

This is unrelated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347

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

[PATCH] D106347: [PoC][RISCV] Encode arch information in a new module flag meatadata 'riscv-isa-features'.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/RISCV/riscv-isa-features.ll:1
+; RUN: llc -o - < %s | FileCheck %s
+; -mattr option would overwrite target-feature and module flag 
riscv-isa-features

Use update_llc_test_checks for these.



Comment at: llvm/test/CodeGen/RISCV/riscv-isa-features.ll:2
+; RUN: llc -o - < %s | FileCheck %s
+; -mattr option would overwrite target-feature and module flag 
riscv-isa-features
+; RUN: llc -o - -mattr=+f,+d < %s | FileCheck %s -check-prefix=ISA-F-D

It's not overriding. It's just that #0 doesn't include -f,-d so applying #0 
atop the default target-features keeps the default f and d features.



Comment at: llvm/test/CodeGen/RISCV/riscv-isa-features.ll:3
+; -mattr option would overwrite target-feature and module flag 
riscv-isa-features
+; RUN: llc -o - -mattr=+f,+d < %s | FileCheck %s -check-prefix=ISA-F-D
+; RUN: llc --filetype=obj -o - < %s | llvm-readelf -A - \

CHECK and ISA-F-D are not consistent with the check prefixes used in other 
RISC-V tests.



Comment at: llvm/test/CodeGen/RISCV/riscv-isa-features.ll:29
+
+attributes #0 = { "target-features"="+64bit,+a,+c,+m"}
+attributes #1 = { "target-features"="+64bit,+a,+c,+d,+f,+m"}

Space before }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347

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

[PATCH] D106347: [PoC][RISCV] Encode arch information in a new module flag meatadata 'riscv-isa-features'.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: 
clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp:9
+// We need to record extension target-feature in riscv-isa-features module 
flag metadata because there is some empty target-features attribute
+// CHECK: !{{[0-9]+}} = !{i32 6, !"riscv-isa-features", !{{[0-9]+}}}
+// CHECK: !{{[0-9]+}} = !{!"+d", !"+f"}

jrtc27 wrote:
> If you're not going to change the representation to be a single string, at 
> least use a FileCheck variable so you can assert that this list is the one 
> referenced by the module attribute above.
No check that the module flags actually includes this?



Comment at: 
clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp:9-10
+// We need to record extension target-feature in riscv-isa-features module 
flag metadata because there is some empty target-features attribute
+// CHECK: !{{[0-9]+}} = !{i32 6, !"riscv-isa-features", !{{[0-9]+}}}
+// CHECK: !{{[0-9]+}} = !{!"+d", !"+f"}
+

If you're not going to change the representation to be a single string, at 
least use a FileCheck variable so you can assert that this list is the one 
referenced by the module attribute above.



Comment at: clang/test/CodeGen/RISCV/riscv-metadata-isa-features.c:15-21
+// BASE-ISA: !{{[0-9]+}} = !{!""}
+// RV32D-ISA: !{{[0-9]+}} = !{!"+d"}
+// RV32FD-ISA: !{{[0-9]+}} = !{!"+d", !"+f"}
+// RV32FDVZFH-ISA: !{{[0-9]+}} = !{!"+d", !"+experimental-v", 
!"+experimental-zfh", !"+f"}
+// RV64D-ISA: !{{[0-9]+}} = !{!"+d"}
+// RV64FD-ISA: !{{[0-9]+}} = !{!"+d", !"+f"}
+// RV32-NEG-D: !{{[0-9]+}} = !{!"-d"}

These need check lines to ensure that these are actually being used for the 
right attribute. Especially `!{!""}`, that could easily be something else that 
you happen to match.



Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:184
 }
-
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {

Don't delete this



Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:187
+dyn_cast_or_null(M.getModuleFlag("riscv-isa-features"));
+if (STI->getFeatureString().empty() && ISAFeatureNodes) {
+  std::vector Features;

This doesn't make sense. The module flag should have been parsed and applied to 
the subtarget long ago. And an empty feature string means no features, not that 
they're missing. The fact that you need to change this code here is a sign that 
code elsewhere is wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347

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

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I can't help but feel the assembly syntax for the register pair instructions 
shouldn't include both registers (perhaps in curly braces). The implicit use of 
the other register when reading the source is rather ugly, and particularly 
hard to remember when the RV64 versions *don't* do that.




Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:442
+
+if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand] &&
+!STI.getFeatureBits()[RISCV::Feature64Bit]) {

The table is called RISCV32POnly but you're checking for Zpsfoperand (whatever 
that mouthful of an extension is). Which is it?



Comment at: llvm/lib/Target/RISCV/RISCV.td:186-188
+   [FeatureExtZpsfoperand,
+FeatureExtZpn,
+FeatureExtZprvsfextra]>;

These aren't correct? RV64 doesn't have Zpsfoperand and RV32 doesn't have 
Zprvsfextra.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:145
+multiclass RVPALU64 funct7, bits<3> funct3, string opcodestr> {
+  let DecoderNamespace = "RISCV32POnly_", Predicates = [HasStdExtZpsfoperand, 
IsRV32] in
+  def "32" : RVPALU64Pair;

These lines look too long to me



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:176-177
+}
+
+
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in

 



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:231
+def CLRS8 : RVPUnary<0b1010111, 0b0, 0b000, "clrs8">,
+Sched<[]>;
+def CLRS16: RVPUnary<0b1010111, 0b01000, 0b000, "clrs16">,

Should probably have a TODO: to add scheduling information for these otherwise 
it'll get lost



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:216
+// Dummy zero register for pairing with X0.
+def ZERO : RISCVReg<0, "0">;
+

Ew, this is a gross quirk of the register pair instructions. ZERO is not a good 
name for it though, that's the ABI name for x0 so already taken and is pretty 
confusing. I assume LLVM doesn't like it if you create a register pair that is 
(X0, X0)?



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:226-228
+def GPRPair : RegisterClass<"RISCV", [untyped], 64, (add GPRPairs)> {
+  let Size = 64;
+}

IMO the register class should be GPR32Pair not GPRPair unless you also make it 
have a sensible interpretation for RV32 (which seems like a waste of time)



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:69
 
+def sub_lo : SubRegIndex<32>;
+def sub_hi : SubRegIndex<32, 32>;

Jim wrote:
> Jim wrote:
> > luismarques wrote:
> > > jrtc27 wrote:
> > > > This assumes RV32, and is not clear it applies to register pairs
> > > What's the best way to address this?
> > sub_lo and sub_hi are only used for GPRPair register class to extract a 
> > register from pair registers on RV32.
> Do you mean that sub_lo and sub_hi only used on RV32? Does it need to rename 
> or ..?
Yes, these should have names that make it clear they're for each half of a 
2*32-bit register pair. Otherwise it sounds like they're the 32-bit hi and lo 
halves of the 64-bit registers on RV64.



Comment at: llvm/test/MC/RISCV/rv64zpn-invalid.s:6-24
+# 16-bit shift
+
+# CHECK-ERROR: immediate must be an integer in the range [0, 15]
+srai16 a0, a1, 21
+
+# CHECK-ERROR: immediate must be an integer in the range [0, 15]
+srai16.u a0, a1, 21

This looks mostly the same as rv32zpn-invalid. The common tests should go in 
rv32zpn-invalid and have RV32 and RV64 RUN lines. Any truly RV32-only tests 
belong in something like rv32zpn-only-invalid. Any truly RV64-only tests can 
stay in rv64zpn-invalid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588

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

[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Oh, technically none of the clang changes belong in this patch. Those are for 
the Clang driver and preprocessor, not the MC layer which is purely llvm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95588

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

[PATCH] D105555: [RISCV][Clang] Compute the default target-abi if it's empty.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:235
+bool Is64Bit = getTriple().getArch() == llvm::Triple::riscv64;
+ABI = llvm::RISCV::computeDefaultABIFromArch(ISAInfo, Is64Bit).str();
+  }

The ISAInfo includes XLen



Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:202
+return llvm::RISCV::computeDefaultABIFromArch(ISAInfo,
+  /*IsRV64=*/XLen == 64);
   }

Ditto (it's literally on the line above!)



Comment at: llvm/lib/Support/TargetParser.cpp:336
+bool Is64Bit) {
+  bool HasD = ISAInfo.hasExtension("d");
+  if (!Is64Bit) {

I'm not personally a fan of having HasD but using ISAInfo.hasExtension("e") in 
the if. I get why it's done, but I think consistency would be better. 
Personally I'd just inline the ISAInfo.hasExtension("d"), it's not worth a 
local variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D10

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

[PATCH] D105555: [RISCV][Clang] Compute the default target-abi if it's empty.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:233
+  // Use the explicitly target-feature to compute default ABI.
+  if (getABI().empty()) {
+bool Is64Bit = getTriple().getArch() == llvm::Triple::riscv64;

I don't think using the getter makes sense. Currently it's entirely equivalent 
to just reading ABI so serves little purpose, but in theory there could be 
additional logic added to it (like asserting it's not empty) that would break 
this. Given you already assume the name of the field below by assigning to it, 
just read ABI directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D10

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

[PATCH] D105555: [RISCV][Clang] Compute the default target-abi if it's empty.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Support/TargetParser.cpp:341-344
+  }
+  if (ISAInfo.hasExtension("d"))
+return "lp64d";
+  return "lp64";

makes me feel more comfortable inside, and also is a bit more like the old code 
where we did explicitly check 32 and 64 (though that instead fell back on the 
next step, rather than asserting).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D10

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

[PATCH] D105555: [RISCV][Clang] Compute the default target-abi if it's empty.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

The commit message needs rewriting to reflect the final patch. Also, 
"Explicitly target-abi encoded in IR is clear than empty target-abi." is wrong, 
it's not about clarity, it's about robustness and correctness.




Comment at: clang/lib/Basic/Targets/RISCV.cpp:232
 
+  // Use the explicitly target-feature to compute default ABI.
+  if (ABI.empty())

I don't think this warrants a comment, and the current grammar is a bit dodgy, 
but if you still want one: "Compute the default ABI based on the target 
features"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D10

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

[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:3-4
+; RUN: llc < %s -mtriple=riscv64 | FileCheck %s
+; RUN: llc < %s -mtriple=riscv64 -align-loops=16 | FileCheck %s 
-check-prefix=ALIGN_16
+; RUN: llc < %s -mtriple=riscv64 -align-loops=32 | FileCheck %s 
-check-prefix=ALIGN_32
+

MaskRay wrote:
> luismarques wrote:
> > Nit: it's a convention of the RISC-V backend codegen tests to wrap the RUN 
> > lines.
> only 86 columns. compiler-rt is even transiting to 100 column.
compiler-rt is not the RISC-V backend :)



Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:10
+; CHECK-LABEL:test:
+; CHECK-NOT:.p2align
+; CHECK:ret

This isn't autogenerated? Also NOT on .p2align isn't great in general, .balign 
and .align don't match that yet could have been emitted.



Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:13
+
+; ALIGN_16-LABEL: test:
+; ALIGN_16: .p2align 4

- not _ in check prefixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106701

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

[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:3-4
+; RUN: llc < %s -mtriple=riscv64 | FileCheck %s
+; RUN: llc < %s -mtriple=riscv64 -align-loops=16 | FileCheck %s 
-check-prefix=ALIGN_16
+; RUN: llc < %s -mtriple=riscv64 -align-loops=32 | FileCheck %s 
-check-prefix=ALIGN_32
+

MaskRay wrote:
> jrtc27 wrote:
> > MaskRay wrote:
> > > luismarques wrote:
> > > > Nit: it's a convention of the RISC-V backend codegen tests to wrap the 
> > > > RUN lines.
> > > only 86 columns. compiler-rt is even transiting to 100 column.
> > compiler-rt is not the RISC-V backend :)
> Wrapping lines here just makes the code less readable.
That's your personal opinion, which I disagree with, and it's not true if your 
terminal isn't wide enough. Going against existing convention in the backend 
tests should only be done with very good reason, and personal opinion is not 
that.



Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:13
+
+; ALIGN_16-LABEL: test:
+; ALIGN_16: .p2align 4

MaskRay wrote:
> jrtc27 wrote:
> > - not _ in check prefixes
> I find that `_` in check prefixes is also popular.
> 
> It has the benefit that `_` cannot conflict with `-NOT` -LABEL` etc.
I have never seen it before and there are zero uses of it in RISC-V CodeGen 
tests. Please conform to the existing style by using -.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106701

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

[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/RISCV/loop-alignment.ll:3-4
+; RUN: llc < %s -mtriple=riscv64 | FileCheck %s
+; RUN: llc < %s -mtriple=riscv64 -align-loops=16 | FileCheck %s 
-check-prefix=ALIGN_16
+; RUN: llc < %s -mtriple=riscv64 -align-loops=32 | FileCheck %s 
-check-prefix=ALIGN_32
+

asb wrote:
> MaskRay wrote:
> > luismarques wrote:
> > > MaskRay wrote:
> > > > jrtc27 wrote:
> > > > > MaskRay wrote:
> > > > > > jrtc27 wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > luismarques wrote:
> > > > > > > > > Nit: it's a convention of the RISC-V backend codegen tests to 
> > > > > > > > > wrap the RUN lines.
> > > > > > > > only 86 columns. compiler-rt is even transiting to 100 column.
> > > > > > > compiler-rt is not the RISC-V backend :)
> > > > > > Wrapping lines here just makes the code less readable.
> > > > > That's your personal opinion, which I disagree with, and it's not 
> > > > > true if your terminal isn't wide enough. Going against existing 
> > > > > convention in the backend tests should only be done with very good 
> > > > > reason, and personal opinion is not that.
> > > > Lines longer than 80-column (in this case just 86) are pretty common 
> > > > among tests. I really hope test/CodeGen/RISCV/ can be more tolerant on 
> > > > this matter.
> > > > 
> > > > Even the Linux scripts/checkpatch.pl has increased the limit to 100 
> > > > because in many cases wrapping lines for strict 80-conformance just 
> > > > harms readability.
> > > > 
> > > > Of course I don't want to waste time arguing on this matter. So if this 
> > > > turns out to be an issue for RISC-V folks, I'll update it to save my 
> > > > time.
> > > > Of course I don't want to waste time arguing on this matter. So if this 
> > > > turns out to be an issue for RISC-V folks, I'll update it to save my 
> > > > time.
> > > 
> > > Personally, I don't particularly care. I don't know if @asb has strong 
> > > feelings about this. If you think it would be beneficial to relax this 
> > > convention please raise the issue on llvm-dev. Let's not keep discussing 
> > > this in every patch touching RISC-V :-)
> > Personally I don't even think the generic case needs to be raised on 
> > llvm-dev:) There are just so many column>80 cases in llvm/test and 
> > clang/test. Actually, If someone wants to enforce the 80-column rule more 
> > rigidly, that probably needs a discussion.
> > 
> > That said, the argument here is about a subdirectory: 
> > llvm/test/CodeGen/RISCV/ ...
> > 
> > 
> I don't have a strong view on this one to be honest. I think I've typically 
> wrapped at 80 columns for these RUN lines after being asked to, but 
> ultimately I think choosing a logical point to split has a greater impact on 
> readability than keeping it strictly to 80 columns.
FWIW I care less about argument lists extending beyond 80 columns, but I do 
think the | is a logical point at which to wrap it if you have a long line and 
keeps things more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106701

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

[PATCH] D105555: [RISCV][Clang] Compute the default target-abi if it's empty.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Commit message needs rewriting still, but the patch looks good to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D10

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:31
+public:
+  RISCVISAInfo() : XLen(0), FLen(0) {}
+

Does Exts need initialising to be empty here? I can never remember



Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:47-52
+  /// Parse RISCV ISA info from arch string.
+  Error parseArchString(StringRef Arch, bool EnableExperimentalExtension,
+bool ExperimentalExtensionVersionCheck = true);
+
+  /// Parse RISCV ISA info from feature vector.
+  void parseFeatures(unsigned XLen, const std::vector &Features);

I wonder, should these be constructors instead? `RISCVISAInfo ISAInfo; 
ISAInfo.parse` doesn't feel very idiomatic C++



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:178
+// order, but after all known standard extensions.
+Rank = AllStdExts.size() + (Ext - 'a');
+  else

Why not just return these directly? Don't need a variable.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:194
+  switch (ExtClass) {
+  case 's':
+HighOrder = 0;

I imagine this needs to change to the new Sm-/Ss- scheme, but I don't know 
which way round those are intended to go



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:197
+break;
+  case 'h':
+HighOrder = 1;

Do we know if this is still the case? Ss- is being used for S-mode extensions 
and Sm- for M-mode extensions, so I'd expect Sh- (or maybe just Ss-) for 
HS-mode extensions, not H-.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:207
+  default:
+assert(false && "Unknown prefix for multi-char extension");
+return -1;

llvm_unreachable



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:214
+  if (ExtClass == 'z')
+LowOrder = singleLetterExtensionRank(ExtName[1]);
+

Why not put this in its switch case?



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:225-229
+  if (LHSLen == 1 && RHSLen != 1)
+return true;
+
+  if (LHSLen != 1 && RHSLen == 1)
+return false;

Don't know if this is better or not, but this is the more compact way to write 
it



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

frasercrmck wrote:
> Not sure why these need to be declared up here.
Yeah, declare the variables at their assignments. Besides, you don't even need 
variables for the single-letter case, just compare the return values of the 
functions directly.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+

kito-cheng wrote:
> jrtc27 wrote:
> > craig.topper wrote:
> > > Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
> > I can't help but feel this is really an array of chars, not a string. We 
> > don't even need the trailing NUL, though double quote syntax is simpler 
> > than curly braces and a bunch of single-quote chars just to save a byte.
> Yeah, it's actually just an array of chars, but we have use find function 
> from StringRef :p
Still not a StringLiteral



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:43
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+{"i", RISCVExtensionVersion{2, 0}, /* Experimental */ false},
+{"e", RISCVExtensionVersion{1, 9}, /* Experimental */ false},

jrtc27 wrote:
> but that's the default so I'd omit it for anything other than the cases where 
> it's true
I do think it would be better to not list Experimental for the non-experimental 
ones


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168

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

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 updated this revision to Diff 362214.
jrtc27 added a comment.

Now only a CC1 option


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105516

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/falways-mem2reg.c
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

Index: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
===
--- llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
+++ llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
@@ -187,6 +187,7 @@
 LibraryInfo = nullptr;
 Inliner = nullptr;
 DisableUnrollLoops = false;
+AlwaysMem2Reg = false;
 SLPVectorize = false;
 LoopVectorize = true;
 LoopsInterleaved = true;
@@ -658,8 +659,11 @@
   MPM.add(createForceFunctionAttrsLegacyPass());
 
   // If all optimizations are disabled, just run the always-inline pass and,
-  // if enabled, the function merging pass.
+  // if enabled, the mem2reg and function merging passes.
   if (OptLevel == 0) {
+if (AlwaysMem2Reg)
+  MPM.add(createPromoteMemoryToRegisterPass());
+
 addPGOInstrPasses(MPM);
 if (Inliner) {
   MPM.add(Inliner);
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -283,6 +283,7 @@
   SLPVectorization = false;
   LoopUnrolling = true;
   ForgetAllSCEVInLoopUnroll = ForgetSCEVInLoopUnroll;
+  AlwaysMem2Reg = false;
   Coroutines = false;
   LicmMssaOptCap = SetLicmMssaOptCap;
   LicmMssaNoAccForPromotionCap = SetLicmMssaNoAccForPromotionCap;
@@ -1931,6 +1932,9 @@
   MPM.addPass(AlwaysInlinerPass(
   /*InsertLifetimeIntrinsics=*/PTO.Coroutines));
 
+  if (PTO.AlwaysMem2Reg)
+MPM.addPass(createModuleToFunctionPassAdaptor(PromotePass()));
+
   if (PTO.MergeFunctions)
 MPM.addPass(MergeFunctionsPass());
 
Index: llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h
===
--- llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h
+++ llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h
@@ -156,6 +156,7 @@
 
   bool DisableTailCalls;
   bool DisableUnrollLoops;
+  bool AlwaysMem2Reg;
   bool CallGraphProfile;
   bool SLPVectorize;
   bool LoopVectorize;
Index: llvm/include/llvm/Passes/PassBuilder.h
===
--- llvm/include/llvm/Passes/PassBuilder.h
+++ llvm/include/llvm/Passes/PassBuilder.h
@@ -107,6 +107,10 @@
   /// is that of the flag: `-forget-scev-loop-unroll`.
   bool ForgetAllSCEVInLoopUnroll;
 
+  /// Tuning option to always run mem2reg regardless of the optimisation level.
+  /// Its default value is false.
+  bool AlwaysMem2Reg;
+
   /// Tuning option to enable/disable coroutine intrinsic lowering. Its default
   /// value is false. Frontends such as Clang may enable this conditionally. For
   /// example, Clang enables this option if the flags `-std=c++2a` or above, or
Index: clang/test/CodeGen/falways-mem2reg.c
===
--- /dev/null
+++ clang/test/CodeGen/falways-mem2reg.c
@@ -0,0 +1,33 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -flegacy-pass-manager -O0 %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -fno-legacy-pass-manager -O0 %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -flegacy-pass-manager -O0 -fno-always-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -fno-legacy-pass-manager -O0 -fno-always-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-NO-MEM2REG %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -flegacy-pass-manager -O0 -falways-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-MEM2REG %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -fno-legacy-pass-manager -O0 -falways-mem2reg %s \
+// RUN:   | FileCheck --check-prefix=O0-MEM2REG %s
+
+// O0-NO-MEM2REG-LABEL: @add(
+// O0-NO-MEM2REG-NEXT:  entry:
+// O0-NO-MEM2REG-NEXT:[[A_ADDR:%.*]] = alloca i32, align 4
+// O0-NO-MEM2REG-NEXT:[[B_ADDR:%.*]] = alloca i32, align 4
+// O0-NO-MEM2REG-NEXT:store i32 [[A:%.*]], i32* [[A_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:store i32 [[B:%.*]], i32* [[B_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:[[TMP0:%.*]] = load i32, i32* [[A_ADDR]], align 4
+// O0-NO-MEM2REG-NEXT:[[TMP1:%.*]] = load i32, i32* [[B_ADDR]

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D105516#2892512 , @rjmccall wrote:

> I agree with Eli: we should decide what the goals are here and then use those 
> goals to decide if we can identify a desirable permanent feature and, if so, 
> what the appropriate name for that feature is.
>
> It sounds like your goal is to get readable assembly that still corresponds 
> fairly literally to your original code, in the sense that the readability of 
> -O0 assembly is often undermined by the sheer amount of code and all the 
> extra, unnecessary work it seems to do.  However, I've found that a lot of 
> the extra -O0 code is not actually from loads and stores to local variables, 
> it's from the -O0 instruction selection and register allocation, which often 
> combine to do very silly things.  Have you looked into whether you get more 
> readable code by just running normal -O0 IR through the non-O0 codegen 
> pipeline?  Because the problem with doing just mem2reg is that that's already 
> a fairly major non-literal change to the code, and at some point it's tricky 
> to say what exactly should be part of this new pipeline; whereas still 
> emitting exactly what the abstract machine says to do, just with less 
> nonsense from fast-isel, is a lot easier to define.

Well, I'm dealing with RISC-V which doesn't have FastISel. And no, running -O0 
IR through a different pipeline is never going to work well without mem2reg 
(and you can't mem2reg -O0 IR without -disable-O0-optnone), so you need at 
least this to work well. Whether or not there's a useful load of additional 
stuff you could do, maybe, but I do think this patch in and of itself makes a 
huge difference, and the limited scope of it is beneficial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105516

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

[PATCH] D92935: Introduce support for PowerPC devices with an Embedded Floating-point APU version 2 (efpu2)

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/PowerPC/spe.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -verify-machineinstrs < %s -mtriple=powerpc-unknown-linux-gnu \
-; RUN:  -mattr=+spe |  FileCheck %s
+; RUN: split-file %s %t
+; RUN: llc -verify-machineinstrs < %t/single.ll 
-mtriple=powerpc-unknown-linux-gnu \

This breaks being able to run update_llc_test_checks.py without first running 
the test itself to generate the files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92935

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

[PATCH] D92935: Introduce support for PowerPC devices with an Embedded Floating-point APU version 2 (efpu2)

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/PowerPC/spe.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -verify-machineinstrs < %s -mtriple=powerpc-unknown-linux-gnu \
-; RUN:  -mattr=+spe |  FileCheck %s
+; RUN: split-file %s %t
+; RUN: llc -verify-machineinstrs < %t/single.ll 
-mtriple=powerpc-unknown-linux-gnu \

jrtc27 wrote:
> This breaks being able to run update_llc_test_checks.py without first running 
> the test itself to generate the files
In fact it breaks it completely, update_llc_test_checks.py doesn't support the 
%t substitutions, and even if it did it's unclear what that even means; should 
it update the CHECK lines in the file with the RUN lines or the files being 
used by FileCheck?

This should have just been manually split.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92935

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

[PATCH] D92935: Introduce support for PowerPC devices with an Embedded Floating-point APU version 2 (efpu2)

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/PowerPC/spe.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -verify-machineinstrs < %s -mtriple=powerpc-unknown-linux-gnu \
-; RUN:  -mattr=+spe |  FileCheck %s
+; RUN: split-file %s %t
+; RUN: llc -verify-machineinstrs < %t/single.ll 
-mtriple=powerpc-unknown-linux-gnu \

jrtc27 wrote:
> jrtc27 wrote:
> > This breaks being able to run update_llc_test_checks.py without first 
> > running the test itself to generate the files
> In fact it breaks it completely, update_llc_test_checks.py doesn't support 
> the %t substitutions, and even if it did it's unclear what that even means; 
> should it update the CHECK lines in the file with the RUN lines or the files 
> being used by FileCheck?
> 
> This should have just been manually split.
Most of this is also entirely unnecessary if you use 
--check-prefixes=CHECK,SPE/EFPU2; update_llc_test_checks.py will merge CHECK 
lines when possible. The only thing that needs splitting is the hwdouble test, 
by just putting it in a separate file (or using sed to preprocess the file so 
SPE sees the test uncommented and EFPU2 sees the test commented out, but that 
seems unnecessary, though does work). I'll write a patch for the former.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92935

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

[PATCH] D105527: libclang.so: Make SONAME independent from LLVM version

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I'm seeing the test fail locally with:

  000d1799 t clang_CompileCommand_getNumMappedSources

being found by grep.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105527

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

[PATCH] D106243: [Utils] Support class template specializations in update_cc_test_checks

[Jessica Clarke 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 rG0e79a94836d7: [Utils] Support class template specializations 
in update_cc_test_checks (authored by jrtc27).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106243

Files:
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp.expected
  clang/test/utils/update_cc_test_checks/explicit-template-instantiation.test
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -33,8 +33,8 @@
 '%clangxx': ['--driver-mode=g++'],
 }
 
-def get_line2spell_and_mangled(args, clang_args):
-  ret = {}
+def get_line2func_list(args, clang_args):
+  ret = collections.defaultdict(list)
   # Use clang's JSON AST dump to get the mangled name
   json_dump_args = [args.clang] + clang_args + ['-fsyntax-only', '-o', '-']
   if '-cc1' not in json_dump_args:
@@ -55,26 +55,37 @@
 
   # Parse the clang JSON and add all children of type FunctionDecl.
   # TODO: Should we add checks for global variables being emitted?
-  def parse_clang_ast_json(node):
+  def parse_clang_ast_json(node, loc, search):
 node_kind = node['kind']
 # Recurse for the following nodes that can contain nested function decls:
 if node_kind in ('NamespaceDecl', 'LinkageSpecDecl', 'TranslationUnitDecl',
- 'CXXRecordDecl'):
+ 'CXXRecordDecl', 'ClassTemplateSpecializationDecl'):
+  # Specializations must use the loc from the specialization, not the
+  # template, and search for the class's spelling as the specialization
+  # does not mention the method names in the source.
+  if node_kind == 'ClassTemplateSpecializationDecl':
+inner_loc = node['loc']
+inner_search = node['name']
+  else:
+inner_loc = None
+inner_search = None
   if 'inner' in node:
 for inner in node['inner']:
-  parse_clang_ast_json(inner)
+  parse_clang_ast_json(inner, inner_loc, inner_search)
 # Otherwise we ignore everything except functions:
 if node_kind not in ('FunctionDecl', 'CXXMethodDecl', 'CXXConstructorDecl',
  'CXXDestructorDecl', 'CXXConversionDecl'):
   return
+if loc is None:
+  loc = node['loc']
 if node.get('isImplicit') is True and node.get('storageClass') == 'extern':
-  common.debug('Skipping builtin function:', node['name'], '@', node['loc'])
+  common.debug('Skipping builtin function:', node['name'], '@', loc)
   return
-common.debug('Found function:', node['kind'], node['name'], '@', node['loc'])
-line = node['loc'].get('line')
+common.debug('Found function:', node['kind'], node['name'], '@', loc)
+line = loc.get('line')
 # If there is no line it is probably a builtin function -> skip
 if line is None:
-  common.debug('Skipping function without line number:', node['name'], '@', node['loc'])
+  common.debug('Skipping function without line number:', node['name'], '@', loc)
   return
 
 # If there is no 'inner' object, it is a function declaration and we can
@@ -88,20 +99,23 @@
   has_body = True
   break
 if not has_body:
-  common.debug('Skipping function without body:', node['name'], '@', node['loc'])
+  common.debug('Skipping function without body:', node['name'], '@', loc)
   return
 spell = node['name']
+if search is None:
+  search = spell
 mangled = node.get('mangledName', spell)
-ret[int(line)-1] = (spell, mangled)
+ret[int(line)-1].append((spell, mangled, search))
 
   ast = json.loads(stdout)
   if ast['kind'] != 'TranslationUnitDecl':
 common.error('Clang AST dump JSON format changed?')
 sys.exit(2)
-  parse_clang_ast_json(ast)
+  parse_clang_ast_json(ast, None, None)
 
-  for line, func_name in sorted(ret.items()):
-common.debug('line {}: found function {}'.format(line+1, func_name), file=sys.stderr)
+  for line, funcs in sorted(ret.items()):
+for func in funcs:
+  common.debug('line {}: found function {}'.format(line+1, func), file=sys.stderr)
   if not ret:
 common.warn('Did not find any functions using', ' '.join(json_dump_args))
   return ret
@@ -222,7 +236,7 @@
  comment_prefix='//', argparse_callback=infer_dependent_args):
 # Build a list of filechecked and non-filechecked RUN lines.
 run_list = []
-line2spell_and_mangled_list = collections.defaultdict(list)
+line2func_list = collections.defaultdict(list)
 
 subs = {
   '%s' : ti.path,
@@ -296,8 +310,8 @@
 
   # Invoke clang -Xclang 

[PATCH] D97606: [Clang interpreter] Avoid storing pointers at unaligned locations

[Jessica Clarke 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 rG40080e7e7f42: [Clang interpreter] Avoid storing pointers at 
unaligned locations (authored by jrtc27).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97606

Files:
  clang/lib/AST/Interp/ByteCodeEmitter.cpp
  clang/lib/AST/Interp/Disasm.cpp
  clang/lib/AST/Interp/Interp.h
  clang/lib/AST/Interp/Program.cpp
  clang/lib/AST/Interp/Program.h
  clang/lib/AST/Interp/Source.h
  clang/utils/TableGen/ClangOpcodesEmitter.cpp

Index: clang/utils/TableGen/ClangOpcodesEmitter.cpp
===
--- clang/utils/TableGen/ClangOpcodesEmitter.cpp
+++ clang/utils/TableGen/ClangOpcodesEmitter.cpp
@@ -124,7 +124,7 @@
 for (size_t I = 0, N = Args.size(); I < N; ++I) {
   OS << "  auto V" << I;
   OS << " = ";
-  OS << "PC.read<" << Args[I]->getValueAsString("Name") << ">();\n";
+  OS << "ReadArg<" << Args[I]->getValueAsString("Name") << ">(S, PC);\n";
 }
 
 // Emit a call to the template method and pass arguments.
@@ -161,8 +161,10 @@
 OS << "  PrintName(\"" << ID << "\");\n";
 OS << "  OS << \"\\t\"";
 
-for (auto *Arg : R->getValueAsListOfDefs("Args"))
-  OS << " << PC.read<" << Arg->getValueAsString("Name") << ">() << \" \"";
+for (auto *Arg : R->getValueAsListOfDefs("Args")) {
+  OS << " << ReadArg<" << Arg->getValueAsString("Name") << ">(P, PC)";
+  OS << " << \" \"";
+}
 
 OS << " << \"\\n\";\n";
 OS << "  continue;\n";
Index: clang/lib/AST/Interp/Source.h
===
--- clang/lib/AST/Interp/Source.h
+++ clang/lib/AST/Interp/Source.h
@@ -44,8 +44,9 @@
   bool operator!=(const CodePtr &RHS) const { return Ptr != RHS.Ptr; }
 
   /// Reads data and advances the pointer.
-  template  T read() {
-T Value = ReadHelper(Ptr);
+  template  std::enable_if_t::value, T> read() {
+using namespace llvm::support;
+T Value = endian::read(Ptr);
 Ptr += sizeof(T);
 return Value;
   }
@@ -54,22 +55,6 @@
   /// Constructor used by Function to generate pointers.
   CodePtr(const char *Ptr) : Ptr(Ptr) {}
 
-  /// Helper to decode a value or a pointer.
-  template 
-  static std::enable_if_t::value, T>
-  ReadHelper(const char *Ptr) {
-using namespace llvm::support;
-return endian::read(Ptr);
-  }
-
-  template 
-  static std::enable_if_t::value, T>
-  ReadHelper(const char *Ptr) {
-using namespace llvm::support;
-auto Punned = endian::read(Ptr);
-return reinterpret_cast(Punned);
-  }
-
 private:
   friend class Function;
 
Index: clang/lib/AST/Interp/Program.h
===
--- clang/lib/AST/Interp/Program.h
+++ clang/lib/AST/Interp/Program.h
@@ -44,6 +44,12 @@
 public:
   Program(Context &Ctx) : Ctx(Ctx) {}
 
+  /// Marshals a native pointer to an ID for embedding in bytecode.
+  unsigned getOrCreateNativePointer(const void *Ptr);
+
+  /// Returns the value of a marshalled native pointer.
+  const void *getNativePointer(unsigned Idx);
+
   /// Emits a string literal among global data.
   unsigned createGlobalString(const StringLiteral *S);
 
@@ -143,6 +149,11 @@
   /// Function relocation locations.
   llvm::DenseMap> Relocs;
 
+  /// Native pointers referenced by bytecode.
+  std::vector NativePointers;
+  /// Cached native pointer indices.
+  llvm::DenseMap NativePointerIndices;
+
   /// Custom allocator for global storage.
   using PoolAllocTy = llvm::BumpPtrAllocatorImpl;
 
Index: clang/lib/AST/Interp/Program.cpp
===
--- clang/lib/AST/Interp/Program.cpp
+++ clang/lib/AST/Interp/Program.cpp
@@ -18,6 +18,21 @@
 using namespace clang;
 using namespace clang::interp;
 
+unsigned Program::getOrCreateNativePointer(const void *Ptr) {
+  auto It = NativePointerIndices.find(Ptr);
+  if (It != NativePointerIndices.end())
+return It->second;
+
+  unsigned Idx = NativePointers.size();
+  NativePointers.push_back(Ptr);
+  NativePointerIndices[Ptr] = Idx;
+  return Idx;
+}
+
+const void *Program::getNativePointer(unsigned Idx) {
+  return NativePointers[Idx];
+}
+
 unsigned Program::createGlobalString(const StringLiteral *S) {
   const size_t CharWidth = S->getCharByteWidth();
   const size_t BitWidth = CharWidth * Ctx.getCharBit();
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -13,8 +13,6 @@
 #ifndef LLVM_CLANG_AST_INTERP_INTERP_H
 #define LLVM_CLANG_AST_INTERP_INTERP_H
 
-#include 
-#include 
 #include "Function.h"
 #include "InterpFrame.h"
 #include "InterpStack.h"
@@ -30,6 +28,9 @@
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llv

[PATCH] D105527: libclang.so: Make SONAME independent from LLVM version

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D105527#2910616 , @tstellar wrote:

> In D105527#2910319 , @jrtc27 wrote:
>
>> I'm seeing the test fail locally (on 
>> ca0fe3447fb85762838468537d93d4ef82c5a1af 
>> ) with:
>>
>>   000d1799 t clang_CompileCommand_getNumMappedSources
>>
>> being found by grep.
>
> What Operating System?

Ubuntu 18.04, amd64, using -fuse-ld=gold


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105527

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

[PATCH] D106974: libcang: Add missing function to libclang.map

[Jessica Clarke via Phabricator via cfe-commits]

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

Thanks; I can confirm the test passes if and only if I apply this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106974

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Ping? I'd really like to get this fixed in 14.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:641
+getASTContext().getTargetInfo().getTriple().getArch();
+if (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be ||
+Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb)

Could use isAArch64 (which then also picks up aarch64_32)



Comment at: clang/lib/AST/ItaniumMangle.cpp:642
+if (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be ||
+Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb)
+  return getStdNamespace();

Could use isARM. Does this also need to care about isThumb, or has that been 
normalised to Arm with a T32 subarch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D118333: [RISCV] Use computeTargetABI from llc as well as clang

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/CodeGen/RISCV/double-mem.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 -mattr=+d -verify-machineinstrs < %s \
+; RUN: llc -mtriple=riscv32 -mattr=+d -target-abi=ilp32 -verify-machineinstrs 
< %s \
 ; RUN:   | FileCheck -check-prefix=RV32IFD %s

For a bunch of these it seems it'd make more sense to just use a hard-float 
ABI. I think it's worthwhile keeping this as NFC test-wise, but don't know if 
it makes sense to update the ABIs first and make this patch smaller or land 
this then go through at a later date so as to not stall this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118333

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

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Jessica Clarke via Phabricator via cfe-commits]

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

Thanks, looks good to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116773: AST: Make getEffectiveDeclContext() a member function of ItaniumMangleContextImpl. NFCI.

[Jessica Clarke via Phabricator via cfe-commits]

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

This makes sense to me but I don't know if you want someone with more authority 
in these parts to review it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116773

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

[PATCH] D116774: AST: Move __va_list tag back to std conditionally on AArch64.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:8555-8556
+// namespace std { struct __va_list {
+NamespaceDecl *NS;
+NS = NamespaceDecl::Create(const_cast(*Context),
+   Context->getTranslationUnitDecl(),

aaron.ballman wrote:
> 
Hm, the advantage of leaving it as it is is then it completely reverts D104830 
(ignoring the added CFI test) rather than being written in a slightly different 
but equivalent way. Don't really care either way myself though, both have their 
merits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D129802: [DRAFT] Implementing new atomic orderings in LLVM and generate barriers for legacy __sync builtins. Support corresponding memory model in outline atomics as well.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I can't obviously see a description of what the additional barriers implied by 
the sync variants is (which should be in an update to LangRef at the very 
least, if not also in the summary itself). Inferring it from the AArch64 
assembly is also difficult, and the RISC-V lowering being identical to the 
non-sync forms confuses me further. I don't particularly want to trawl through 
the web of mailing list posts to try and find out which emails have the right 
information and which aren't relevant, either.




Comment at: llvm/include/llvm-c/Core.h:360
+  operations. */
+  LLVMAtomicOrderingSyncAcquire = 8, /**< Acquire with additional barrier. */
+  LLVMAtomicOrderingSyncRelease = 9, /**< Release with additional barrier. */

These comments are not particularly insightful



Comment at: llvm/include/llvm/IR/Instructions.h:660
+  /// Release requests from the AtomicOrdering. A SequentiallyConsistent and
+  /// StrongSequetiallyConsistent operations would remain
+  /// SequentiallyConsistent.

You mean SyncSequentiallyConsistent?



Comment at: llvm/include/llvm/IR/Instructions.h:670
 case AtomicOrdering::Monotonic:
   return AtomicOrdering::Monotonic;
 case AtomicOrdering::AcquireRelease:

Are these correct? Without a clear description of what SyncFoo add over and 
above Foo it's hard to know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129802

___
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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Does GCC allow this or not? Because this strikes me as a bad idea at first 
sight…


Repository:
  rG LLVM Github Monorepo

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] D125272: [clang] Add -fcheck-new support

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:3
+// RUN: %clang_cc1 -fcheck-new -triple x86_64-linux-gnu -S -disable-O0-optnone 
\
+// RUN: -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
+

MaskRay wrote:
> Please remove `opt`. Optimizations should be tested in the llvm/ layer, not 
> in clang codegen.
> 
> If utils/update_cc_test_checks.py does not generate good looking IR, write it 
> by hand.
Disagree. This is a pattern that is extremely useful for testing Clang's IR 
generation whilst cleaning up the alloca so the IR is readable. This is not 
about the generated checks, this is about alloca/load/store everywhere being a 
pain to inspect, whereas a simple mem2reg cleans this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125272

___
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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

If you still want to pursue this, discussion belongs at 
https://github.com/riscv-non-isa/riscv-toolchain-conventions, not here, since 
it's an interface shared by Clang and GCC and the two should be consistent


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] D125272: [clang] Add -fcheck-new support

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang -fcheck-new -emit-llvm -S %s -o - -O2 | FileCheck %s
+

Do you really want -O2 or do you just want to run mem2reg to eliminate all the 
alloca noise?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125272

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

[PATCH] D125272: [clang] Add -fcheck-new support

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang -fcheck-new -emit-llvm -S %s -o - -O2 | FileCheck %s
+

heatd wrote:
> jrtc27 wrote:
> > Do you really want -O2 or do you just want to run mem2reg to eliminate all 
> > the alloca noise?
> I added -O2 because I was testing with it, since if -fcheck-new doesn't work, 
> it's way more noticeable, as the nullptr check gets optimized out; if it 
> works, the branching is pretty visible and shows exactly what the option 
> does. Also, to eliminate all the noise :) I can definitely remove -O2 though, 
> if you want to.
Thinking about this more, I think the branching complicates matters. All that 
optimisation happens in LLVM IR land, but you're just touching Clang CodeGen. 
Is `int *foo() { return new int; }` checking the attributes on the call to 
_Znwm not enough (and with mem2reg just to clean it up so the function body is 
trivial)?

Also you should specify a target triple as this will just be 
LLVM_DEFAULT_TARGET_TRIPLE and change both the type for new (i32 vs i64) and 
its mangling (j vs m for unsigned int vs unsigned long, and *-windows-msvc is 
totally different), right?



Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:9
+// CHECK:   3:
+// CHECK-NEXT:store i32 48879, ptr [[TMP1]], align 4, !tbaa 
[[TBAA5:![0-9]+]]
+// CHECK-NEXT:br label [[TMP4]]

Another advantage of not using -O2 is you lose the TBAA noise



Comment at: clang/test/Driver/clang_f_opts.c:73
 
-
 // RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof %s 2>&1 | FileCheck 
-check-prefix=CHECK-AUTO-PROFILE %s

Hmmm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125272

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

[PATCH] D125272: [clang] Add -fcheck-new support

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:1
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang -fcheck-new --target=x86_64-linux-gnu -emit-llvm -S %s -o - | 
opt -S -mem2reg | FileCheck %s

Not sure why they were checked before, but you'll need this to actually get the 
attributes checked


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125272

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

[PATCH] D125272: [clang] Add -fcheck-new support

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGenCXX/fcheck-new.cpp:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang -fcheck-new --target=x86_64-linux-gnu -emit-llvm -S %s -o - | 
opt -S -mem2reg | FileCheck %s
+

Not sure how this doesn't need -Xclang -disable-O0-optnone to work?..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125272

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

[PATCH] D125947: [RISCV] Add default ABI for archs with only F extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

It's currently this way in order to be compatible with GCC. Changing this 
requires consensus from both toolchains to ensure compatibility is preserved. 
See https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/13 for 
some discussion on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125947

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

[PATCH] D125947: [RISCV] Add default ABI for archs with only F extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Also, the tests where you have codegen changes rather than preserving a 
soft-float ABI should probably be put up for review separately by adding an 
explicit hard single-float ABI, as those seem worthwhile for reducing noise


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125947

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

[PATCH] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D157331#4654353 , @aaron.ballman 
wrote:

> In D157331#4654265 , @mstorsjo 
> wrote:
>
>> This change broke building a recent version of gettext. Gettext uses gnulib 
>> for polyfilling various utility functions. Since not long ago, these 
>> functions internally use ``, 
>> https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f.
>>  If the toolchain doesn't provide the header, gnulib provides a fallback 
>> version of its own: 
>> https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h
>>
>> Now since Clang provides it since this commit, gnulib doesn't provide their 
>> own fallback, but goes on to use `ckd_add`. This fails with Clang's 
>> `` implementation, since gnulib isn't built in C23 mode but 
>> still wants to use ``:
>>
>>   i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. 
>> -I../../../gettext-runtime/gnulib-lib -I..  -I../intl 
>> -I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 
>> -DDEPENDS_ON_LIBINTL=1 
>> -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual 
>> -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef 
>> -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion 
>> -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits 
>> -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o 
>> libgrt_a-malloca.o `test -f 'malloca.c' || echo 
>> '../../../gettext-runtime/gnulib-lib/'`malloca.c
>>   ../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to 
>> undeclared function 'ckd_add'; ISO C99 and later do not support implicit 
>> function declarations [-Wimplicit-function-declaration]
>>  53 |   if (!ckd_add (&nplus, n, plus) && !xalloc_oversized (nplus, 1))
>> |^
>>   1 error generated.
>>   make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1
>>
>> It seems like GCC's version of this header exposes the functionality even if 
>> compiled in an older language mode: 
>> https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h
>>
>> Would you consider changing the Clang version to do the same? Otherwise we 
>> would need to convince gnulib to not try to use/polyfill this header unless 
>> gnulib itself is compiled in C23 mode.
>
> Well that's a fun situation. The identifiers exposed by the header are not in 
> a reserved namespace and they're macros, which is usually not something we'd 
> want to expose in older language modes because of the risk of breaking code. 
> But in this case, there's an explicit opt-in to a brand-new header file so it 
> shouldn't break code to expose the contents in older language modes. But I 
> expect there to be changes to this file in C2y, and those additions could 
> break code if we're not careful.
>
> CC @jrtc27 @jyknight @efriedma for opinions from others, but my thinking is 
> that we should expose the contents in earlier language modes. We support the 
> builtin in those modes, and the user is explicitly asking for the header to 
> be included -- it seems pretty rare for folks to want the header to be empty, 
> especially given that `__has_include` will report "sure do!". I don't think 
> there's a conformance issue here.

Yes. That's also what we do for stdalign.h, which is the same case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D159546: [OpenMP 5.2] Initial parsing and semantic analysis support for 'step' modifier on 'linear' clause

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1352
+def err_omp_multiple_step_or_linear_modifier : Error<
+  "multiple %select{'step size'|'linear modifier'}0 found in linear clause">; 
 def warn_pragma_expected_colon_r_paren : Warning<

This trailing whitespace broke CI's formatting check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159546

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

[PATCH] D159546: [OpenMP 5.2] Initial parsing and semantic analysis support for 'step' modifier on 'linear' clause

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1352
+def err_omp_multiple_step_or_linear_modifier : Error<
+  "multiple %select{'step size'|'linear modifier'}0 found in linear clause">; 
 def warn_pragma_expected_colon_r_paren : Warning<

jrtc27 wrote:
> This trailing whitespace broke CI's formatting check
(I've gone and fixed it in 0ab694754e3722f7edbd8b3ad23ac0b312515d3b)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159546

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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

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

This is currently incompatible with the save/restore libcalls, and I don't 
think there's any way to avoid that (the restore libcall both loads ra and 
jumps to it). We should ensure combining them give an error.




Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:36-37
+
+  // Do not save RA to SCS if it's not saved to regular stack, i.e.
+  // RA is not subject to overwritten.
+  std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo();





Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:43
+
+  // Get shadow call stack pointer register.
+  Register SCSPReg = RISCVABI::getSCSPReg();

Pointless comment; remove



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:46
+
+  // Emit an error message and bail out.
+  if (!STI.isRegisterReservedByUser(SCSPReg)) {

Pointless comment; remove



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:53
+
+  DebugLoc DL = MI != MBB.end() ? MI->getDebugLoc() : DebugLoc();
+

This should be passed in as an argument IMO (same for the epilogue) given the 
standard prologue/epilogue code already has a DebugLoc lying around.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:59-60
+  // Store return address to shadow call stack
+  // sw   ra, 0(s2)
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(IsRV64 ? RISCV::SD : RISCV::SW))





Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:60
+  // sw   ra, 0(s2)
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(IsRV64 ? RISCV::SD : RISCV::SW))

Is it intended that the shadow call stack grows *up* unlike the normal stack?



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:79-80
+
+  // Do not restore RA from SCS if it's not saved to regular stack, i.e.
+  // RA is not subject to overwritten.
+  std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo();

No need to repeat ourselves.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:102-103
+  // Load return address from shadow call stack
+  // addi s2, s2, -4
+  // lw   ra, 0(s2)
+  BuildMI(MBB, MI, DL, TII->get(RISCV::ADDI))

Although in fact you have both a bug and a minor performance issue with this, 
and it should be:

```
  // l[w|d] ra, [-4|-8](s2)
  // addi   s2, s2, -[4|8]
```

Then there's no read-after-write dependency chain, which is better for 
out-of-order cores.

The bug is that, currently, you risk a signal handler clobbering your SCS slot 
in between the two instructions, since you deallocate the frame before you read 
from it. Will be rare in practice, but a possibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414

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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:86
+
+  // Get shadow call stack pointer register.
+  Register SCSPReg = RISCVABI::getSCSPReg();

Pointless comment; remove



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:89
+
+  // Emit an error message and bail out.
+  if (!STI.isRegisterReservedByUser(SCSPReg)) {

Pointless comment; remove


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414

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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:38
+  // Do not save RA to the SCS if it's not saved to the regular stack,
+  // i.e. RA is not at risk of being to overwritten.
+  std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo();





Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:60
+  // sw   ra, 0(s2)
+  // addi s2, s2, 4
+  BuildMI(MBB, MI, DL, TII->get(IsRV64 ? RISCV::SD : RISCV::SW))

zzheng wrote:
> jrtc27 wrote:
> > Is it intended that the shadow call stack grows *up* unlike the normal 
> > stack?
> No. Which direction the SCS grows on is trivial.
> 
> The memory area hosting SCS is independent of the regular stack; and it's 
> provided by the runtime.
> mmap/malloc returns the low address of newly mapped/allocated area. Making 
> the SCS growing down requires the runtime to return upper bound of the SCS. 
> On AArch64, the SCS grows up as well.
Ok. Wasn't saying there was anything wrong with it, was just something that 
jumped out at me. Having it grow up makes more sense (the only real advantage 
to the normal stack growing down these days is that doing aligned allocations 
is slightly cheaper).



Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:79-80
+
+  // Do not restore RA from SCS if it's not saved to regular stack, i.e.
+  // RA is not subject to overwritten.
+  std::vector &CSI = MF.getFrameInfo().getCalleeSavedInfo();

jrtc27 wrote:
> No need to repeat ourselves.
This still holds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added subscribers: trasz, dim, jrtc27.
jrtc27 added a comment.
Herald added a subscriber: dang.

This has significantly regressed FreeBSD's performance with the new version of 
Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since 
FreeBSD currently compiles its kernel with -O whenever debug symbols are 
enabled[1], this results in all its `static inline` helper functions not being 
inlined at all, a pattern that is common in the kernel, used for things like 
`get_curthread` and the atomics implementations.

[1] This is a dubious decision made r140400 in 2005 to provide "truer debugger 
stack traces" (well, before then there was ping-ponging between -O and -O2 
based on concerns around correctness vs performance, but amd64 is an exception 
that has always used -O2 since r127180 it seems). Given that GCC will inline at 
-O, at least these days, the motivation seems to no longer exist, and compiling 
a kernel at anything other than -O2 (or maybe -O3) seems like a silly thing to 
do, but nevertheless it's what is currently done.

Cc: @dim @trasz


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D84414: [RISCV] Support Shadow Call Stack

[Jessica Clarke via Phabricator via cfe-commits]

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

Yes I think everything's been addressed now (though if I keep looking over it I 
might start nit-picking even more :)).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84414

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D79916#2279812 , @Bdragon28 wrote:

> In D79916#2279045 , @jrtc27 wrote:
>
>> This has significantly regressed FreeBSD's performance with the new version 
>> of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and 
>> since FreeBSD currently compiles its kernel with -O whenever debug symbols 
>> are enabled[1] (which, of course, is almost always true), this results in 
>> all its `static inline` helper functions not being inlined at all, a pattern 
>> that is common in the kernel, used for things like `get_curthread` and the 
>> atomics implementations.
>>
>> [1] This is a dubious decision made in r140400 in 2005 to provide "truer 
>> debugger stack traces" (well, before then there was ping-ponging between -O 
>> and -O2 based on concerns around correctness vs performance, but amd64 is an 
>> exception that has always used -O2 since r127180 it seems). Given that GCC 
>> will inline at -O, at least these days, the motivation seems to no longer 
>> exist, and compiling a kernel at anything other than -O2 (or maybe -O3) 
>> seems like a silly thing to do, but nevertheless it's what is currently done.
>>
>> Cc: @dim @trasz
>
> This is actually SUCH a bad idea that a kernel built with -O will *not work 
> at all* on 32 bit powerpc platforms (presumably due to allocating stack 
> frames in the middle of assembly fragments in the memory management that are 
> supposed to be inlined at all times.) I had to hack kern.pre.mk to rquest -O2 
> at all times just to get a functioning kernel.

Well, -O0, -O1, -O2 and -O should all produce working kernels, and any cases 
where they don't are compiler bugs (or kernel bugs if they rely on UB) that 
should be fixed, not worked around by tweaking the compiler flags in a fragile 
way until you get the behaviour relied on. Correctness and performance are very 
different issues here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D79916#2279863 , @Bdragon28 wrote:

> In D79916#2279816 , @jrtc27 wrote:
>
>> In D79916#2279812 , @Bdragon28 
>> wrote:
>>
>>> In D79916#2279045 , @jrtc27 wrote:
>>>
 This has significantly regressed FreeBSD's performance with the new 
 version of Clang. It seems Clang does not inline functions at -O1, unlike 
 GCC, and since FreeBSD currently compiles its kernel with -O whenever 
 debug symbols are enabled[1] (which, of course, is almost always true), 
 this results in all its `static inline` helper functions not being inlined 
 at all, a pattern that is common in the kernel, used for things like 
 `get_curthread` and the atomics implementations.

 [1] This is a dubious decision made in r140400 in 2005 to provide "truer 
 debugger stack traces" (well, before then there was ping-ponging between 
 -O and -O2 based on concerns around correctness vs performance, but amd64 
 is an exception that has always used -O2 since r127180 it seems). Given 
 that GCC will inline at -O, at least these days, the motivation seems to 
 no longer exist, and compiling a kernel at anything other than -O2 (or 
 maybe -O3) seems like a silly thing to do, but nevertheless it's what is 
 currently done.

 Cc: @dim @trasz
>>>
>>> This is actually SUCH a bad idea that a kernel built with -O will *not work 
>>> at all* on 32 bit powerpc platforms (presumably due to allocating stack 
>>> frames in the middle of assembly fragments in the memory management that 
>>> are supposed to be inlined at all times.) I had to hack kern.pre.mk to 
>>> rquest -O2 at all times just to get a functioning kernel.
>>
>> Well, -O0, -O1, -O2 and -O should all produce working kernels, and any cases 
>> where they don't are compiler bugs (or kernel bugs if they rely on UB) that 
>> should be fixed, not worked around by tweaking the compiler flags in a 
>> fragile way until you get the behaviour relied on. Correctness and 
>> performance are very different issues here.
>
> As an example:
>
>   static __inline void
>   mtsrin(vm_offset_t va, register_t value)
>   {
>   
>   __asm __volatile ("mtsrin %0,%1; isync" :: "r"(value), "r"(va));
>   }
>
> This code is used in the mmu when bootstrapping the cpu like so:
>
>   for (i = 0; i < 16; i++)
>   mtsrin(i << ADDR_SR_SHFT, kernel_pmap->pm_sr[i]);
>   powerpc_sync();
>   
>   sdr = (u_int)moea_pteg_table | (moea_pteg_mask >> 10);
>   __asm __volatile("mtsdr1 %0" :: "r"(sdr));
>   isync();
>   
>   tlbia();
>
> During the loop there, we are in the middle of programming the MMU segment 
> registers in real mode, and is supposed to be doing all work out of 
> registers. (and powerpc_sync() and isync() should be expanded to their single 
> assembly instruction, not a function call. The whole point of calling those 
> is that we are in an inconsistent hardware state and need to sync up before 
> continuing execution)
>
> If there isn't a way to force inlining, we will have to change to using 
> preprocessor macros in cpufunc.h.

There is, it's called `__attribute__((always_inline))` and supported by both 
GCC and Clang. But at -O0 you'll still have register allocation to deal with, 
so really that code is just fundamentally broken and should not be written in 
C. There is no way for you to guarantee stack spills are not used, it's way out 
of scope for C.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

(and FreeBSD has an `__always_inline` in sys/sys/cdef.s like `__inline`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D79916#2279871 , @Bdragon28 wrote:

> In D79916#2279866 , @jrtc27 wrote:
>
>> In D79916#2279863 , @Bdragon28 
>> wrote:
>>
>>> In D79916#2279816 , @jrtc27 wrote:
>>>
 In D79916#2279812 , @Bdragon28 
 wrote:

> In D79916#2279045 , @jrtc27 
> wrote:
>
>> This has significantly regressed FreeBSD's performance with the new 
>> version of Clang. It seems Clang does not inline functions at -O1, 
>> unlike GCC, and since FreeBSD currently compiles its kernel with -O 
>> whenever debug symbols are enabled[1] (which, of course, is almost 
>> always true), this results in all its `static inline` helper functions 
>> not being inlined at all, a pattern that is common in the kernel, used 
>> for things like `get_curthread` and the atomics implementations.
>>
>> [1] This is a dubious decision made in r140400 in 2005 to provide "truer 
>> debugger stack traces" (well, before then there was ping-ponging between 
>> -O and -O2 based on concerns around correctness vs performance, but 
>> amd64 is an exception that has always used -O2 since r127180 it seems). 
>> Given that GCC will inline at -O, at least these days, the motivation 
>> seems to no longer exist, and compiling a kernel at anything other than 
>> -O2 (or maybe -O3) seems like a silly thing to do, but nevertheless it's 
>> what is currently done.
>>
>> Cc: @dim @trasz
>
> This is actually SUCH a bad idea that a kernel built with -O will *not 
> work at all* on 32 bit powerpc platforms (presumably due to allocating 
> stack frames in the middle of assembly fragments in the memory management 
> that are supposed to be inlined at all times.) I had to hack kern.pre.mk 
> to rquest -O2 at all times just to get a functioning kernel.

 Well, -O0, -O1, -O2 and -O should all produce working kernels, and any 
 cases where they don't are compiler bugs (or kernel bugs if they rely on 
 UB) that should be fixed, not worked around by tweaking the compiler flags 
 in a fragile way until you get the behaviour relied on. Correctness and 
 performance are very different issues here.
>>>
>>> As an example:
>>>
>>>   static __inline void
>>>   mtsrin(vm_offset_t va, register_t value)
>>>   {
>>>   
>>>   __asm __volatile ("mtsrin %0,%1; isync" :: "r"(value), "r"(va));
>>>   }
>>>
>>> This code is used in the mmu when bootstrapping the cpu like so:
>>>
>>>   for (i = 0; i < 16; i++)
>>>   mtsrin(i << ADDR_SR_SHFT, kernel_pmap->pm_sr[i]);
>>>   powerpc_sync();
>>>   
>>>   sdr = (u_int)moea_pteg_table | (moea_pteg_mask >> 10);
>>>   __asm __volatile("mtsdr1 %0" :: "r"(sdr));
>>>   isync();
>>>   
>>>   tlbia();
>>>
>>> During the loop there, we are in the middle of programming the MMU segment 
>>> registers in real mode, and is supposed to be doing all work out of 
>>> registers. (and powerpc_sync() and isync() should be expanded to their 
>>> single assembly instruction, not a function call. The whole point of 
>>> calling those is that we are in an inconsistent hardware state and need to 
>>> sync up before continuing execution)
>>>
>>> If there isn't a way to force inlining, we will have to change to using 
>>> preprocessor macros in cpufunc.h.
>>
>> There is, it's called `__attribute__((always_inline))` and supported by both 
>> GCC and Clang. But at -O0 you'll still have register allocation to deal 
>> with, so really that code is just fundamentally broken and should not be 
>> written in C. There is no way for you to guarantee stack spills are not 
>> used, it's way out of scope for C.
>
> Is there a way to have always_inline and unused at the same time? I tried 
> using always_inline and it caused warnings in things that used *parts* of 
> cpufunc.h.

Both `__attribute__((always_inline)) __attribute__((unused))` and 
`__attribute__((always_inline, unused))` work, but really you should use 
`__always_inline __unused` in FreeBSD (which will expand to the former).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D79916#2279875 , @jrtc27 wrote:

> In D79916#2279871 , @Bdragon28 wrote:
>
>> In D79916#2279866 , @jrtc27 wrote:
>>
>>> In D79916#2279863 , @Bdragon28 
>>> wrote:
>>>
 In D79916#2279816 , @jrtc27 wrote:

> In D79916#2279812 , @Bdragon28 
> wrote:
>
>> In D79916#2279045 , @jrtc27 
>> wrote:
>>
>>> This has significantly regressed FreeBSD's performance with the new 
>>> version of Clang. It seems Clang does not inline functions at -O1, 
>>> unlike GCC, and since FreeBSD currently compiles its kernel with -O 
>>> whenever debug symbols are enabled[1] (which, of course, is almost 
>>> always true), this results in all its `static inline` helper functions 
>>> not being inlined at all, a pattern that is common in the kernel, used 
>>> for things like `get_curthread` and the atomics implementations.
>>>
>>> [1] This is a dubious decision made in r140400 in 2005 to provide 
>>> "truer debugger stack traces" (well, before then there was ping-ponging 
>>> between -O and -O2 based on concerns around correctness vs performance, 
>>> but amd64 is an exception that has always used -O2 since r127180 it 
>>> seems). Given that GCC will inline at -O, at least these days, the 
>>> motivation seems to no longer exist, and compiling a kernel at anything 
>>> other than -O2 (or maybe -O3) seems like a silly thing to do, but 
>>> nevertheless it's what is currently done.
>>>
>>> Cc: @dim @trasz
>>
>> This is actually SUCH a bad idea that a kernel built with -O will *not 
>> work at all* on 32 bit powerpc platforms (presumably due to allocating 
>> stack frames in the middle of assembly fragments in the memory 
>> management that are supposed to be inlined at all times.) I had to hack 
>> kern.pre.mk to rquest -O2 at all times just to get a functioning kernel.
>
> Well, -O0, -O1, -O2 and -O should all produce working kernels, and any 
> cases where they don't are compiler bugs (or kernel bugs if they rely on 
> UB) that should be fixed, not worked around by tweaking the compiler 
> flags in a fragile way until you get the behaviour relied on. Correctness 
> and performance are very different issues here.

 As an example:

   static __inline void
   mtsrin(vm_offset_t va, register_t value)
   {
   
   __asm __volatile ("mtsrin %0,%1; isync" :: "r"(value), "r"(va));
   }

 This code is used in the mmu when bootstrapping the cpu like so:

   for (i = 0; i < 16; i++)
   mtsrin(i << ADDR_SR_SHFT, kernel_pmap->pm_sr[i]);
   powerpc_sync();
   
   sdr = (u_int)moea_pteg_table | (moea_pteg_mask >> 10);
   __asm __volatile("mtsdr1 %0" :: "r"(sdr));
   isync();
   
   tlbia();

 During the loop there, we are in the middle of programming the MMU segment 
 registers in real mode, and is supposed to be doing all work out of 
 registers. (and powerpc_sync() and isync() should be expanded to their 
 single assembly instruction, not a function call. The whole point of 
 calling those is that we are in an inconsistent hardware state and need to 
 sync up before continuing execution)

 If there isn't a way to force inlining, we will have to change to using 
 preprocessor macros in cpufunc.h.
>>>
>>> There is, it's called `__attribute__((always_inline))` and supported by 
>>> both GCC and Clang. But at -O0 you'll still have register allocation to 
>>> deal with, so really that code is just fundamentally broken and should not 
>>> be written in C. There is no way for you to guarantee stack spills are not 
>>> used, it's way out of scope for C.
>>
>> Is there a way to have always_inline and unused at the same time? I tried 
>> using always_inline and it caused warnings in things that used *parts* of 
>> cpufunc.h.
>
> Both `__attribute__((always_inline)) __attribute__((unused))` and 
> `__attribute__((always_inline, unused))` work, but really you should use 
> `__always_inline __unused` in FreeBSD (which will expand to the former).

But also you really should not get warnings for unused static functions in 
included headers, only ones defined in the C source file itself. We'd have 
countless warnings in the kernel across all architectures otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D79916#2279901 , @Bdragon28 wrote:

> In D79916#2279884 , @jrtc27 wrote:
>
>> But also you really should not get warnings for unused static functions in 
>> included headers, only ones defined in the C source file itself. We'd have 
>> countless warnings in the kernel across all architectures otherwise.
>
> I agree. But that's what it is doing when using always_inline in combination 
> with -Wunused-function.
>
> There is currently no real usage of always_inline in system headers though, 
> so maybe I'm just the first to complain about it?

We use them in CheriBSD and have no such issues that I've ever noticed. When 
was the last time you checked (and what compiler)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D79916: Map -O to -O1 instead of -O2

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D79916#2279987 , @MaskRay wrote:

> Several previous comments are FreeBSD specific. To we clang developers, the 
> concrete request is
>
>> Given that GCC will inline at -O, at least these days, ...
>
> right? I think this makes sense, especially when `inline` is explicitly 
> specified... This appears to be related to some -O1 work @echristo is working 
> on.
>
>   // gcc -O1 and g++ -O1 inline `foo`. Note that in C99 mode, `extern int 
> foo` is needed to ask the compiler to provide an external definition.
>   // clang -O1 and clang++ -O1 do not inline `foo`
>   inline int foo(int a) {
> return a + a;
>   }
>   
>   int bar(int a, int b) {
> return foo(a + b);
>   }

Yes, `inline` should certainly be inlined, but also non-`inline` things should 
too. Perhaps not so aggressively, but there's no good reason not to, really, it 
can be a big win with a simple transformation. GCC seems to inline even those:

  # echo 'static void foo(void) { __asm__ __volatile__ ("#asdf"); } void 
bar(void) { foo(); } void baz(void) { foo(); foo(); }' | gcc -x c - -o - -S -O1
.file   ""
.text
.globl  bar
.type   bar, @function
  bar:
  .LFB1:
.cfi_startproc
  #APP
  # 1 "" 1
#asdf
  # 0 "" 2
  #NO_APP
ret
.cfi_endproc
  .LFE1:
.size   bar, .-bar
.globl  baz
.type   baz, @function
  baz:
  .LFB2:
.cfi_startproc
  #APP
  # 1 "" 1
#asdf
  # 0 "" 2
  # 1 "" 1
#asdf
  # 0 "" 2
  #NO_APP
ret
.cfi_endproc
  .LFE2:
.size   baz, .-baz
.ident  "GCC: (Debian 10.2.0-8) 10.2.0"
.section.note.GNU-stack,"",@progbits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79916

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

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

If someone cares about pthread_create they might wish to follow up on my 
https://reviews.llvm.org/D58531, which I filed early last year to permit 
pthread_create to have a proper type in the syntax. It will likely need 
rebasing, but probably isn't that much work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77491

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||

arichardson wrote:
> jrtc27 wrote:
> > 
> I feel like moving it here could in theory miss some cases. Also the 
> condition is already barely comprehensible (I didn't attempt to understand 
> which special cases all these conditions are for) and I don't feel like 
> making it more complex.
> 
> If clang-format has identified that this */& token is a pointer/reference, 
> and the next token is something that can be paresed as a pointer qualifier, 
> shouldn't we trust the parser and simply look at the format option? It also 
> IMO makes the code slightly easier to understand.
It's a series of `A || B || C || ...`, and you just added an `if (X) return 
true;` above, so changing it to `A || B || C || X || ...` is entirely 
equivalent, no need to understand what any of the other expressions in the 
giant OR are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||

arichardson wrote:
> jrtc27 wrote:
> > arichardson wrote:
> > > jrtc27 wrote:
> > > > 
> > > I feel like moving it here could in theory miss some cases. Also the 
> > > condition is already barely comprehensible (I didn't attempt to 
> > > understand which special cases all these conditions are for) and I don't 
> > > feel like making it more complex.
> > > 
> > > If clang-format has identified that this */& token is a 
> > > pointer/reference, and the next token is something that can be paresed as 
> > > a pointer qualifier, shouldn't we trust the parser and simply look at the 
> > > format option? It also IMO makes the code slightly easier to understand.
> > It's a series of `A || B || C || ...`, and you just added an `if (X) return 
> > true;` above, so changing it to `A || B || C || X || ...` is entirely 
> > equivalent, no need to understand what any of the other expressions in the 
> > giant OR are.
> Never mind, I thought your suggestion it was inside nested parens. It's 
> equivalent just avoids the braces.
Uh of course with `||` added on the end of that new sub-expression, managed to 
omit that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

[Jessica Clarke via Phabricator via cfe-commits]

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

GCC on Linux defines `__sparc_v9__` even with `-m32`. I don't know what Solaris 
does but please don't break other operating systems just because Solaris has 
broken headers that conflate the CPU and the ABI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86621

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

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

And notably _doesn't_ define the V8 macros, which this patch then reintroduces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86621

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

[PATCH] D86621: [clang][Sparc] Default to -mcpu=v9 for SparcV8 on Solaris

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Actually, `__sparcv8` is only for V8; if you have 32-bit V9 on Solaris it 
defines `__sparcv8plus` _instead_:

  jrtc27@gcc-solaris11:~$ /opt/solarisstudio12.4/bin/cc -E - -xarch=v9 -m32 
-xdumpmacros &1 | grep sparc
  #define __sparcv8plus 1
  #define __sparc 1
  #define sparc 1

In fact, -xarch=v9 + -m32 is a bit weird because -xarch=v9 implies -m64 so the 
argument order matters, and the modern way to do it is (if you read the man 
page, -xarch=sparc means V9 and -xarch=v9 is an alias for -m64 -xarch=sparc...):

  jrtc27@gcc-solaris11:~$ /opt/solarisstudio12.4/bin/cc -E - -m32 -xarch=sparc 
-xdumpmacros &1 | grep sparc
  #define __sparcv8plus 1
  #define __sparc 1
  #define sparc 1

(gcc211 on the GCC compile farm; any open-source developer can register, it's 
not specific to GCC developers despite the name)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86621

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

[PATCH] D86782: [clang-format] Allow configuring list of macros that map to attributes

[Jessica Clarke via Phabricator via cfe-commits]

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

The documentation currently shows `__capability` being included, but from 
looking at this patch does the configuration file not append (which I think 
makes sense, at least for `__capability`) rather than replace?




Comment at: clang/docs/ClangFormatStyleOptions.rst:776
+
+TypeQualifiers: ['__capability', '__output', '__ununsed']
+





Comment at: clang/include/clang/Format/Format.h:599
+  /// \code{.yaml}
+  ///   TypeQualifiers: ['__capability', '__output', '__ununsed']
+  /// \endcode




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86782

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D52050#2441133 , @glaubitz wrote:

> In D52050#2441094 , @hvdijk wrote:
>
>> I've been able to check what Ubuntu 20.10 offers in terms of x32 support. 
>> Its kernel supports x32 binaries, it provides x32 versions of core system 
>> libraries in separate packages (e.g. libc6-x32, libx32stdc++6, libx32z1), 
>> and it provides a compiler that targets x32 by default 
>> (gcc-x86-64-linux-gnux32).
>
> I did that, too. In fact, Ubuntu is identical to Debian in this regard as 
> both the Ubuntu and the Debian gcc packages are maintained by the same 
> maintainer (Matthias Klose whom I also happen to know personally) who first 
> uploads these packages to Debian unstable, then syncs to Ubuntu.
>
> However:
>
>> These Ubuntu packages do not use the Debian/Ubuntu multiarch approach: the 
>> packages are completely independent of the corresponding x64 and i386 
>> versions with separate names, and nothing in Ubuntu installs any libraries 
>> into any /lib/x86_64-linux-gnux32-like path. They are intended to allow x32 
>> cross compilation on an Ubuntu system, not intended to act as a basis for 
>> running an x32 Ubuntu system. This appears to be very different from 
>> Debian's x32 support. That said, cross-compiled binaries do run on Ubuntu 
>> and it should be possible to build an x32-native LLVM with the 
>> Ubuntu-provided toolchain.
>
> Well, Debian has both as - as I already mentioned - the gcc packages in 
> Debian and Ubuntu are the same. The only difference is that Ubuntu does not 
> provide an x32 port so there is no possibility to install Ubuntu x32 packages 
> in the commonly known MultiArch manner.
>
> Since MultiArch and the -cross packages are somewhat redundant, I'm not so 
> sure yet which approach to address this issue would be best. I will talk to 
> Matthias regarding this.

What gets done currently for i386? That suffers potentially the same problem 
given both /usr/lib/gcc/x86_64-linux-gnu/$V/32 and 
/usr/lib/gcc/i386-linux-gnu/$V are possible locations for an i386 GCC toolchain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

i.e. can we not just support both approaches and prefer x86_64-linux-gnux32 if 
it exists?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Firstly, please generate your diffs with full context (-U with a 
sufficiently-large number). Secondly, can we avoid having to do a bunch of 
duplication with some clever use of multiclasses for F/D/Zfh and pseudos? 
Though maybe it's small enough that the duplication is easier to reason about 
than an obfuscated abstracted version.

Also, do you not need more predicates? You can't just assume all of F, D and 
Zfh exist.

As for Zfinx itself, well, the idea is fine, but I really detest the way it's 
being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not 
work_ so it is not an _extension_. Instead it should really be a set of 
separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and 
Zfhinx, but apparently asking code that complies with a ratified standard to 
change itself in order to not break when a new extension is introduced is a-ok 
in the RISC-V world.




Comment at: llvm/test/MC/RISCV/rv32zfh-invalid.s:33
-# Integer registers where FP regs are expected
-fadd.h a2, a1, a0 # CHECK: :[[@LINE]]:8: error: invalid operand for instruction
-

These need to stay, but presumably instead as errors about Zfinx not being 
enabled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93298

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

[PATCH] D88393: [cfe][M68k] (Patch 7/8) Basic Clang support

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Basic/Targets/M68k.cpp:38-50
+  // M68k pointers are always 32 bit wide even for 16 bit cpus
+  Layout += "-p:32:32";
+
+  // M68k integer data types
+  Layout += "-i8:8:8-i16:16:16-i32:32:32";
+
+  // FIXME no floats at the moment

If we're in SysV psABI land, then the stack is 32-bit aligned.

If we're in actual ABI used by everyone out there, i.e. GCC's default, then 
it's only 16-bit aligned, and your integer types also have the wrong alignment, 
so you will get ABI incompatibility with GCC-built binaries as provided by 
distributions like Debian.



Comment at: clang/lib/Basic/Targets/M68k.cpp:63
+.Case("generic", CK_68000)
+.Case("M68000", CK_68000)
+.Case("M68010", CK_68010)

GCC's -mcpu excludes any M prefix and is just the number.



Comment at: clang/lib/Basic/Targets/M68k.cpp:77-79
+  Builder.defineMacro("M68k");
+  Builder.defineMacro("__M68k__");
+  Builder.defineMacro("__M68K__");

Where are these coming from? GCC only defines `__m68k__`.


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

https://reviews.llvm.org/D88393

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

[PATCH] D88394: [Driver][M68k] (Patch 8/8) Add driver support for M68k

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2089
+  static const char *const M68kTriples[] = {
+  "m68k-linux-gnu", "m68k-unknown-linux-gnu", "m68k-suse-linux"};
+

rengolin wrote:
> The front-end supports FreeBSD, too.
Aren't these arrays only used on multiarch systems so irrelevant for BSDs? 
There aren't FreeBSD triples listed for X86/X86_64 for example.


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

https://reviews.llvm.org/D88394

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D52050#2466874 , @glaubitz wrote:

> In D52050#2441164 , @glaubitz wrote:
>
>> In D52050#2441141 , @jrtc27 wrote:
>>
>>> What gets done currently for i386? That suffers potentially the same 
>>> problem given both /usr/lib/gcc/x86_64-linux-gnu/$V/32 and 
>>> /usr/lib/gcc/i386-linux-gnu/$V are possible locations for an i386 GCC 
>>> toolchain.
>>
>> Very good suggestion. I will look into that tomorrow. Thanks for the pointer!
>
> I have been wrapping my head around this for some time and I have run into 
> one problem trying to apply the suggested approach.
>
> The problem is that I don't know how to tell whether I'm on an x86_64 system 
> or an x32 system there is no ```case llvm::Triple::x32:``` which would be 
> needed here (we have it for x86).
>
> x32 takes the switch case for x86_64, so x32 and x86_64 targeting x32 are 
> identical.
>
> However, that's not the same as whether we're on an x86_64 system or on an 
> x32 system determines which GNU triplet to use and which include and library 
> search paths are our primary ones.

But `Triple.getEnvironment()` will give you `llvm::Triple::GNUX32` that you can 
check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D91442: [clang][Driver] Handle risvc in Baremetal.cpp.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

We use pure LLVM toolchains so improving support for that out of the box is 
good in my books. However, I do worry this is going to cause friction for a lot 
of people using LLVM for RISC-V; my understanding is that most use LLVM with a 
GNU sysroot and binutils, and so this looks like it will break their setups. Is 
there anything that can be done to automatically detect such cases? What does 
Arm do here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91442

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