[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] 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] 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] 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] 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 (, 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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Thanks, my concerns in the tests have been addressed


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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/C/C2x/n2683_2.c:5-6
+#include 
+#include 
+// CHECK-LABEL: define dso_local void @test_add_overflow_to64(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] {

jrtc27 wrote:
> UTC won't insert such a blank line if it doesn't exist in the input
Not done despite being marked done



Comment at: clang/test/Headers/stdckdint.c:3
+// RUN: %clang_cc1 -triple=x86_64 -emit-llvm -verify -std=c23 %s -o - | 
FileCheck %s
+// expected-no-diagnostics
+#include 




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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/C/C2x/n2683_2.c:5-6
+#include 
+#include 
+// CHECK-LABEL: define dso_local void @test_add_overflow_to64(
+// CHECK-SAME: ) #[[ATTR0:[0-9]+]] {

UTC won't insert such a blank line if it doesn't exist in the input


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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D157331#4653224 , @aaron.ballman 
wrote:

> In D157331#4653222 , @jrtc27 wrote:
>
>> One more thought: we need to specify a triple for the tests as otherwise 
>> it'll use whatever default you have configured in LLVM, which will affect 
>> the exact code generated (especially wrt argument and return lowering), no?
>
> Now that we're using the script (which generates a lot more CHECK lines), I 
> think we will have to add a triple, good catch. `x86_64-unknown-unknown` 
> seems like it would be a reasonable triple (but I defer to @jrtc27 if she's 
> got other ideas).

Yeah, generally "pick your favourite x86_64 triple". In RISC-V land we 
generally go for a bare riscv64 as the triple for anything that's not 
OS-specific and just pick up whatever the bare-metal ABI is (normally close to 
whatever Linux and BSDs do), but historically x86_64 seems to be a bit more of 
a mix of everything (bare, linux-gnu, unknown-unknown, something apple-y). I'd 
lean towards just x86_64, but it doesn't hugely matter here.


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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

One more thought: we need to specify a triple for the tests as otherwise it'll 
use whatever default you have configured in LLVM, which will affect the exact 
code generated (especially wrt argument and return lowering), no?


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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/C/C2x/n2683_2.c:14
+// CHECK:store i64 %2, ptr %[[RES64]], align 8
+// CHECK:%frombool = zext i1 %1 to i8
+// CHECK:store i8 %frombool, ptr %[[FLAG_ADD:.*]], align 1

You missed this one. But please just use the script, it makes everyone's lives 
easier, both in the present (no need to worry that you've forgotten anything) 
and in the future (should Clang's code generation change it's much easier to 
rerun the script rather than hand-edit every test; which also includes 
downstreams that alter behaviour, which I am very familiar with thanks to my 
day job).


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] D157331: [clang] Implement C23

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/C/C2x/n2683_2.c:7-15
+// CHECK:%result64 = alloca i64, align 8
+// CHECK:%flag_add = alloca i8, align 1
+// CHECK:store i64 0, ptr %result64, align 8
+// CHECK:%0 = call { i64, i1 } @llvm.sadd.with.overflow.i64(i64 
2147483647, i64 1)
+// CHECK:%1 = extractvalue { i64, i1 } %0, 1 
+// CHECK:%2 = extractvalue { i64, i1 } %0, 0
+// CHECK:store i64 %2, ptr %result64, align 8

aaron.ballman wrote:
> Sorry for not spotting this sooner, but when emitting LLVM IR, these `%ident` 
> identifiers are sometimes replaced with different text depending on how the 
> bot is set up. e.g., one bot may use `%result64` while another bot may decide 
> to name it `%0`. The way we usually handle this is to use a feature from 
> FileCheck that lets you name a regex pattern and then use that. I showed some 
> examples of how to capture the name and how to use the name as a replacement 
> above, but I'll leave it to you to do the rest of the changes. You should do 
> this for anything starting with `%` in this file and in 
> clang/test/Headers/stdckdint.c.
Or just use update_cc_test_checks.py and let it do that for you? These tests 
don't look like they do anything weird that the UTC scripts won't like.


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] D149867: [Clang][M68k] Add Clang support for the new M68k_RTD CC

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Posting on GitHub loses all context; please don't do that. Whose review are you 
looking for? You've addressed my feedback and you had approval from Aaron on 
the prior version, with the latest version just being a few minor tweaks to 
address the couple of comments I had left on the approved version.


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

https://reviews.llvm.org/D149867

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

[PATCH] D145214: [TSAN] add support for riscv64

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: compiler-rt/lib/tsan/rtl/CMakeLists.txt:5
 append_list_if(COMPILER_RT_HAS_MSSE4_2_FLAG -msse4.2 TSAN_RTL_CFLAGS)
-append_list_if(SANITIZER_LIMIT_FRAME_SIZE -Wframe-larger-than=530
+append_list_if(SANITIZER_LIMIT_FRAME_SIZE -Wframe-larger-than=656
TSAN_RTL_CFLAGS)

hiraditya wrote:
> dvyukov wrote:
> > vitalybuka wrote:
> > > Maybe this one is not needed after 
> > > b31bd6d8046d01a66aa92993bacb56b115a67fc5
> > Yes, is this needed? What function does have larger frame?
> > If we increase the limit, other arches will slowly slinetly degrade too.
> yeah probably not needed anymore. but if we need this we can just change this 
> only for RISC-V?
I would rather diagnose and fix whatever makes RISC-V code generation less 
efficient


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145214

___
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 inline comments.



Comment at: clang/lib/Headers/stdckdint.h:12
+#define __STDCKDINT_H
+
+/* C23 7.20.1 Defines several macros for performing checked integer 
arithmetic*/

pirama wrote:
> aaron.ballman wrote:
> > Should a hosted build attempt to do an include_next into the system library 
> > and then fall back to the compiler builtins, or should we treat this like 
> > stdbool.h where the compiler always wins? CC @jyknight @jrtc27 @efriedma 
> > 
> > My intuition is that we want to include_next in case the system has better 
> > facilities than the compiler does.
> The `include_next` question is still open.  Any preference here?
> 
> IMO, since the standard explicitly delegates to the compiler builtin when 
> available, we may not need to `include_next` - unless there are other 
> conventions around this.
FreeBSD has just added an implementation because Clang doesn’t have one. GCC 
will include_next if _LIBC_STDCKDINT_H isn’t defined and there is a header to 
include. I don’t really know why that exists to be honest, I don’t see a use 
for it and view this like foointrin.h headers. But probably we should be 
compatible with that. I don’t see a use case for it in FreeBSD though.


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] D158624: [RISCV] Implement multiVersionSortPriority

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I don't think you can just use any arbitrary function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158624

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

[PATCH] D149867: [Clang][M68k] Add Clang support for the new M68k_RTD CC

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGen/mrtd.c:2
+// RUN: %clang_cc1 -mrtd -triple i386-unknown-unknown -std=c89 -emit-llvm -o - 
%s 2>&1 | FileCheck --check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -mrtd -triple m68k-unknown-unknown -std=c89 -emit-llvm -o - 
%s 2>&1 | FileCheck --check-prefixes=CHECK,M68k %s
 

Capitalise CHECK prefixes



Comment at: clang/test/CodeGen/mrtd.c:10
+// X86: call x86_stdcallcc i32 @bar(
+#ifndef mc68000
   bar(arg);

Uh, this shouldn't be defined in ISO C; GCC's using builtin_define_std, so you 
should only get `__mc68000` and `__mc68000__` for ISO C, i.e. using 
`DefineStd("mc68000")` in libClangBasic and let it add the underscored variants 
needed


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

https://reviews.llvm.org/D149867

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

[PATCH] D152793: [RISCV] Add MC layer support for Zicfiss.

[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/lib/Target/RISCV/RISCVInstrInfoZicfiss.td:1
+//=== RISCVInstrInfoZicfiss.td - RISC-V CFG -*- tablegen 
-*===//
+//

CFG?



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZicfiss.td:48-49
+let Uses = [SSP], hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
+def SSLOAD :
+  RVInstI<0b100, OPC_SYSTEM, (outs GPRRA:$rd), (ins), "ssload", "$rd"> {
+  let rs1 = 0;

This is some strange wrapping; normally we'd put RVInstI on the same line and 
wrap some of the arguments, but if you really want to wrap then indent by 4 
spaces and put the colon on the lower line, that's what the predominant style is



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZicfiss.td:62
+let Uses = [SSP], hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+def SSPINC : RVInstI<0b100, OPC_SYSTEM, (outs), (ins uimm5nonzero:$rs1),
+ "sspinc", "$rs1"> {

This is how I'd expect the earlier instructions to be formatted, FWIW... please 
be consistent



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZicfiss.td:94
+def C_SSPOPCHK : RVC_SSInst<0b00101, GPRX5, "c.sspopchk">;
+}// Predicates = [HasStdExtZicfiss, HasStdExtC]

Missing space


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152793

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

[PATCH] D157663: [Driver] Default riscv*-linux* to -fdebug-default-version=4

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Also Haiku I guess, as another OS that has a somewhat working RISC-V port but 
falls back on ToolChain's default implementation. So maybe this just belongs in 
the default implementation, given all the ones that need modifying are the ones 
that end up there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157663

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

[PATCH] D157663: [Driver] Default riscv*-linux* to -fdebug-default-version=4

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

RISCVToolchain also needs an override


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157663

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

[PATCH] D155668: [RISCV] Upgrade Zvfh version to 1.0 and move out of experimental state.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Uh, why are there clang/test/Sema/aarch64* tests full of RISC-V extension 
names? That's not right at all. One of them is coming from 
https://reviews.llvm.org/D135011, I haven't traced the rest back, but that's 
clearly wrong. The test looks to be a copy of the RISC-V one with tweaks to 
change riscv(64) to arm/aarch64, and `  -target-feature +sve` (with two spaces) 
added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155668

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

[PATCH] D156565: Diagnose use of VLAs in C++ by default

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Given GCC defines GNU C++ and regards this as a feature (unless you use things 
like -pedantic to ask for ISO C++), does it make sense to enable this for GNU 
C++? Every deviation from GCC is a point of friction for adopting Clang over 
GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

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

[PATCH] D155326: [Clang][lld][RISCV] Passing 'mattr' to lld/llvmgold

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Why do you believe this is better than encoding it in the module's IR like the 
ABI?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155326

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

- Asm tests are a no in Clang
- You seem to have some crazy long lists that I doubt need to be that long for 
test coverage (nor do you need so many variations, surely)
- CHECL is not CHECK
- You have some very strange spacing in CHECK lines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151730

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Isn't multiversioning a separate thing that builds on top of per-function 
target attributes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151730

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

[PATCH] D154397: [Driver] Default to -ftls-model=initial-exec on SerenityOS

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

This is pretty dodgy, I don't think it belongs upstream


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154397

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

[PATCH] D154285: [clang] Remove CGBuilderTy::CreateElementBitCast

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 accepted this revision.
jrtc27 added a comment.

Thanks for the updated diff (would have also been happy with having a new 
commit *before* this one that does the non-trivial changes, but this works too 
and can be followed up with the cleanups you were making if you want to)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/CGBuilder.h:158
 
   /// This method is to be deprecated. Use `Address::withElementType` instead.
+  [[deprecated("Use `Address::withElementType` instead.")]]

jrtc27 wrote:
> JOE1994 wrote:
> > nikic wrote:
> > > JOE1994 wrote:
> > > > 
> > > This is a private method, so simply delete it instead of deprecating.
> > It seems like this method is listed as "public" member functions in 
> > https://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CGBuilderTy.html .
> > 
> > I see the `public` specifier on line 50.
> It's public in the sense of its access specifier, i.e. that it can be used 
> outside of CGBuiltin, but it's private in the sense that this header is in 
> clang/lib/CodeGen and thus only used within Clang itself, not exposed as a 
> Clang API, so if Clang isn't using it any more, nothing is.
Uh, CGBuilderTy, not CGBuiltin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/CGBuilder.h:158
 
   /// This method is to be deprecated. Use `Address::withElementType` instead.
+  [[deprecated("Use `Address::withElementType` instead.")]]

JOE1994 wrote:
> nikic wrote:
> > JOE1994 wrote:
> > > 
> > This is a private method, so simply delete it instead of deprecating.
> It seems like this method is listed as "public" member functions in 
> https://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CGBuilderTy.html .
> 
> I see the `public` specifier on line 50.
It's public in the sense of its access specifier, i.e. that it can be used 
outside of CGBuiltin, but it's private in the sense that this header is in 
clang/lib/CodeGen and thus only used within Clang itself, not exposed as a 
Clang API, so if Clang isn't using it any more, nothing is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3896
 llvm::Type *OrigBaseElemTy = Addr.getElementType();
-Addr = Builder.CreateElementBitCast(Addr, Int8Ty);
 

JOE1994 wrote:
> barannikov88 wrote:
> > jrtc27 wrote:
> > > This one isn't a direct substitution, although appears to be refactoring 
> > > the code to not need withElementType in the first place? Probably best to 
> > > separate that change out from the very mechanical substitution.
> > It will be difficult to find all such places later.
> @jrtc27 Thank you for taking the time to review this revision.
> 
> This isn't a direct substitution.
> In this revision overall, I did refactoring to get rid of unnecessary calls 
> to `withElementType()` when possible.
> 
> Since these are relatively small & local refactorings, I'd prefer to keep 
> them in this revision.
> But if you're strongly against including such refactorings in this revision, 
> I can separate them out to a new revision.
It's easier to review when the non-trivial changes (direct substitution) are 
kept separate from the trivial ones. That way it's also extremely clear what's 
intended to be different vs what was accidentally made different. You *at 
least* need to fix the commit message to say that you're not just replacing the 
in-tree uses, you're also refactoring some uses to not need it (currently your 
commit message is in direct contradiction with the diff), but the fact that the 
commit then does two things that can be reasonably separated out suggests that 
they should be. And I would much rather see that happen, especially from the 
perspective of maintaining a significant downstream fork.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3954
 Function *F = CGM.getIntrinsic(Intrinsic::eh_sjlj_setjmp);
-Buf = Builder.CreateElementBitCast(Buf, Int8Ty);
 return RValue::get(Builder.CreateCall(F, Buf.getPointer()));

Missed `Buf = Buf.withElementType(Int8Ty);`? According to the comment above, 
Buf is a `void **` not a `void *`/`char *`.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3896
 llvm::Type *OrigBaseElemTy = Addr.getElementType();
-Addr = Builder.CreateElementBitCast(Addr, Int8Ty);
 

This one isn't a direct substitution, although appears to be refactoring the 
code to not need withElementType in the first place? Probably best to separate 
that change out from the very mechanical substitution.



Comment at: clang/lib/CodeGen/CGObjCRuntime.cpp:111
+  Address Addr =
+  Address(V, llvm::Type::getIntNTy(CGF.getLLVMContext(), 
Info->StorageSize),
+  Alignment);

Ditto



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:813
-  // Cast to char*.
-  Base = Builder.CreateElementBitCast(Base, CGF.Int8Ty);
-

Ditto



Comment at: clang/lib/CodeGen/Targets/CSKY.cpp:66
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+return Address(CGF.Builder.CreateLoad(VAListAddr),
+   CGF.ConvertTypeForMem(Ty), SlotSize);

Ditto



Comment at: clang/lib/CodeGen/Targets/RISCV.cpp:465
   if (isEmptyRecord(getContext(), Ty, true)) {
-Address Addr = Address(CGF.Builder.CreateLoad(VAListAddr),
-   getVAListElementType(CGF), SlotSize);
-Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
-return Addr;
+return Address(CGF.Builder.CreateLoad(VAListAddr),
+   CGF.ConvertTypeForMem(Ty), SlotSize);

Ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D154050: [Clang][RISCV] Fix RISC-V vector / SiFive intrinsic inclusion in SemaLookup

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:201
 
-void RISCVIntrinsicManagerImpl::InitIntrinsicList() {
+void RISCVIntrinsicManagerImpl::ConstructRVVIntrinsics(
+ArrayRef Recs, IntrinsicKind K) {

As far as I can tell this refactoring was entirely gratuitous, and perhaps a 
regression given the function is only ever used by InitIntrinsicList and thus 
can be hidden as a lambda?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154050

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

[PATCH] D147875: [clang][Diagnostics] Show line numbers when printing code snippets

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D147875#4448545 , @hans wrote:

> I noticed that at least for some cases of `-Wformat`, the line numbers on the 
> left seem to be off: https://github.com/llvm/llvm-project/issues/63524

That's because DisplayLineNo refers to the line of the diagnostic, not the 
first source line, and then counts up. What's the reason for DisplayLineNo in 
the first place? Given the printed line comes from LineNo, surely we should be 
using LineNo instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147875

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

[PATCH] D153170: [RISCV] Sort the extensions in SupportedExtensions and SupportedExperimentalExtensions.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Can we have an expensive check that the table is sorted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153170

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

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D153008#4425238 , @abel-bernabeu 
wrote:

> In D153008#4424821 , @jrtc27 wrote:
>
>> Clang tests should not compile to asm. You want an IR test.
>
> Jessica, are there any exceptions for tests is under CodeGen/RISCV intended 
> to exercise the assembly parser?
>
> I have just written a test that reproduces the way I manually test the 
> feature.

No. You can test the assembly parser just as easily from IR.

I'll also note that your assembly isn't particularly minimal, which it should 
be, unless the `add zero, %[dst], %[dst]` lines are doing something I'm not 
aware of?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153008

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

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

[Jessica Clarke via Phabricator via cfe-commits]

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

Clang tests should not compile to asm. You want an IR test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153008

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

[PATCH] D149867: [M68k] Add Clang support for the new M68k_RTD CC

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D149867#4387189 , @glaubitz wrote:

> In D149867#4386271 , @jrtc27 wrote:
>
>> I disagree. Being experimental doesn't mean you should do the wrong thing. 
>> Reusing stdcall in the frontend is ugly, pollutes non-m68k code paths (doing 
>> your own thing _avoids_ that and makes the experimental backend get out of 
>> the way, even) and introduces a bug where you can write garbage code and 
>> have it compile rather than be rejected like it should be.
>
> Maybe we can get the TLS stuff merged first before dealing with this more 
> complex change?

They're totally independent, so nothing's blocking that being merged. Plus this 
isn't a complex change to make, just a bunch of copy pasta.


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

https://reviews.llvm.org/D149867

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

[PATCH] D149867: [M68k] Add Clang support for the new M68k_RTD CC

[Jessica Clarke via Phabricator via cfe-commits]

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

I disagree. Being experimental doesn't mean you should do the wrong thing. 
Reusing std call in the frontend is ugly, pollutes non-m68k code paths (doing 
your own thing _avoids_ that and makes the experimental backend get out of the 
way, even) and introduces a bug where you can write garbage code and have it 
compile rather than be rejected like it should be.

Add your own calling convention like everyone else.


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

https://reviews.llvm.org/D149867

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

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D150875#4353484 , @erichkeane 
wrote:

> In D150875#4353467 , @jrtc27 wrote:
>
>> We heavily rely on this extension in CheriBSD via `__typeof__((*(p))) * 
>> __capability` as we want to be able to take any pointer, including to an 
>> array or function that needs to undergo decay to be an actual pointer, and 
>> turn it into a `__capability`-qualified one. Presumably you're saying we 
>> should instead use `__typeof__((0, (p))) __capability` as an uglier 
>> alternative way to force decay?
>
> Do you do that in C++, or just C?  Note that this does NOT change the 
> behavior in C.  In C++ I'd probably just suggest using `std::decay`.

In practice just C (kernel-only macro), though ideally the macro would still 
work in C++ (at least on CheriBSD; C++-based OSes will differ, but can of 
course do their own thing).


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

https://reviews.llvm.org/D150875

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

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

We heavily rely on this extension in CheriBSD via `__typeof__((*(p))) * 
__capability` as we want to be able to take any pointer, including to an array 
or function that needs to undergo decay to be an actual pointer, and turn it 
into a `__capability`-qualified one. Presumably you're saying we should instead 
use `__typeof((0, (p)))__ __capability` as an uglier alternative way to force 
decay?


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

https://reviews.llvm.org/D150875

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

[PATCH] D132247: [clang] Fix emitVoidPtrVAArg for non-zero default alloca address space

[Jessica Clarke via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG74f207883bc5: [clang] Fix emitVoidPtrVAArg for non-zero 
default alloca address space (authored by jrtc27).

Changed prior to commit:
  https://reviews.llvm.org/D132247?vs=454033=522295#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132247

Files:
  clang/lib/CodeGen/TargetInfo.cpp


Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -401,8 +401,10 @@
 
   // Cast the address we've calculated to the right type.
   llvm::Type *DirectTy = CGF.ConvertTypeForMem(ValueTy), *ElementTy = DirectTy;
-  if (IsIndirect)
-DirectTy = DirectTy->getPointerTo(0);
+  if (IsIndirect) {
+unsigned AllocaAS = CGF.CGM.getDataLayout().getAllocaAddrSpace();
+DirectTy = DirectTy->getPointerTo(AllocaAS);
+  }
 
   Address Addr = emitVoidPtrDirectVAArg(CGF, VAListAddr, DirectTy, DirectSize,
 DirectAlign, SlotSizeAndAlign,


Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -401,8 +401,10 @@
 
   // Cast the address we've calculated to the right type.
   llvm::Type *DirectTy = CGF.ConvertTypeForMem(ValueTy), *ElementTy = DirectTy;
-  if (IsIndirect)
-DirectTy = DirectTy->getPointerTo(0);
+  if (IsIndirect) {
+unsigned AllocaAS = CGF.CGM.getDataLayout().getAllocaAddrSpace();
+DirectTy = DirectTy->getPointerTo(AllocaAS);
+  }
 
   Address Addr = emitVoidPtrDirectVAArg(CGF, VAListAddr, DirectTy, DirectSize,
 DirectAlign, SlotSizeAndAlign,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149867: [M68k] Add Clang support for the new M68k_RTD CC

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

So GCC gives me:

  warning: ‘stdcall’ attribute directive ignored [-Wattributes]

when trying to use `__attribute__((stdcall))` on m68k, which matches the fact 
it's only mentioned in the manage for the x86 option.

Interestingly it seems to ICE during expand when I use -mrtd though...




Comment at: clang/lib/Basic/Targets/M68k.cpp:258
+  // The M68k_RTD calling convention.
+  case CC_X86StdCall:
+return CCCR_OK;

Uhh



Comment at: clang/lib/CodeGen/CGCall.cpp:51
   default: return llvm::CallingConv::C;
-  case CC_X86StdCall: return llvm::CallingConv::X86_StdCall;
+  case CC_X86StdCall:
+return CGM.getTriple().getArch() == llvm::Triple::m68k

Ditto...



Comment at: clang/test/CodeGen/mrtd.c:4
 
-// CHECK: mrtd.c:10:3: warning: function with no prototype cannot use the 
stdcall calling convention
+// CHECK: mrtd.c:13:3: warning: function with no prototype cannot use the 
stdcall calling convention
 

Ew... this should be using -verify and `// expected-warning {{...}}`, then the 
line number is relative to the comment's location. Or, really, it should be in 
the Sema test...

Plus is it correct to call this stdcall on m68k? The GCC manpage only mentions 
it in the x86 option description, not the m68k one.


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

https://reviews.llvm.org/D149867

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

[PATCH] D147875: [clang][Diagnostics] Show line numbers when printing code snippets

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Frontend/TextDiagnostic.cpp:1121-1138
+static unsigned getNumDisplayWidth(unsigned N) {
+  if (N < 10)
+return 1;
+  if (N < 100)
+return 2;
+  if (N < 1'000)
+return 3;

kwk wrote:
> This function screamed at me to be generalized so I gave it a try: 
> https://gist.github.com/kwk/7e408065ea291e49fea4a83cf90a9cdf
I don't think I want floating point arithmetic in my compiler... Something like:

```
unsigned L, M;
for (L = 1U, M = 10U; N >= M && M != ~0U; ++L)
M = (M > ((~0U) / 10U)) ? (~0U) : (M * 10U);

return (L);
```

is the generalised form (without all the redundant parentheses I added during 
debugging).


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

https://reviews.llvm.org/D147875

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/test/MC/RISCV/rv32zcmt-invalid.s:9
+
+# CHECK-ERROR: error: immediate must be an integer in the range [0, 255]
+cm.jalt 256

This is wrong; the immediate must be in the range [32, 255]. This needs to be 
enforced in the assembler and the error needs to reflect that. We also need 
tests that check these cases (cm.jt with something in [32, 255] and cm.jalt 
with something in [0, 31]).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133863

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

[PATCH] D148817: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/Driver/riscv-default-build-attributes.s:3
+// RUN: %clang --target=riscv64 -### %s 2>&1 \
+// RUN:| FileCheck %s -check-prefix CHECK-ENABLED
+

craig.topper wrote:
> jrtc27 wrote:
> > jrtc27 wrote:
> > > `=` for these
> > Also aren't these indented 1 too many spaces?
> Cleary I should not have copied ARM's test :(
Yeah... :( ARM and AArch64 tests can be pretty iffy in my experience


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148817

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

[PATCH] D148817: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Some more... hopefully spotted everything this time, sorry




Comment at: clang/test/Driver/riscv-default-build-attributes.s:1
+ Enabled by default for assembly
+// RUN: %clang --target=riscv64 -### %s 2>&1 \





Comment at: clang/test/Driver/riscv-default-build-attributes.s:3
+// RUN: %clang --target=riscv64 -### %s 2>&1 \
+// RUN:| FileCheck %s -check-prefix CHECK-ENABLED
+

`=` for these



Comment at: clang/test/Driver/riscv-default-build-attributes.s:3
+// RUN: %clang --target=riscv64 -### %s 2>&1 \
+// RUN:| FileCheck %s -check-prefix CHECK-ENABLED
+

jrtc27 wrote:
> `=` for these
Also aren't these indented 1 too many spaces?



Comment at: clang/test/Driver/riscv-default-build-attributes.s:20
+// CHECK-ENABLED: "-riscv-add-build-attributes"
+// expected-warning {{argument unused during compilation: 
'-mno-default-build-attributes'}}

There aren't any uses of -verify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148817

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

[PATCH] D148817: [RISCV] Add Tag_RISCV_arch attribute by default when using clang as an assembler.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Surprised that we didn't already do this; seems like a bit of an oversight.




Comment at: clang/test/Driver/riscv-default-build-attributes.s:1
+// Enabled by default for assembly
+// RUN: %clang --target=riscv64 -### %s 2>&1 \

Do we use /// for non-lit/FileCheck lines like we use ;; in IR tests?



Comment at: clang/test/Driver/riscv-default-build-attributes.s:11
+
+
+// Option ignored C/C++ (since we always emit hardware and ABI build attributes

Gap is a bit odd



Comment at: clang/test/Driver/riscv-default-build-attributes.s:12
+
+// Option ignored C/C++ (since we always emit hardware and ABI build attributes
+// during codegen).

missing "for"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148817

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

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering for RV64

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:277
+void RISCVAsmPrinter::LowerKCFI_CHECK(const MachineInstr ) {
+  assert(STI->is64Bit() && "KCFI_CHECK requires RV64");
+

What about this code relies on it being 64-bit?



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1898
+def KCFI_CHECK : Pseudo<
+  (outs), (ins GPRJALR:$ptr, i32imm:$type), []>, Sched<[]>;
+}

This formatting clearly doesn't match the surrounding code



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1901
+
+
 /// Simple optimization

Don't introduce two blank lines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148385

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

[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering for RV64

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I can't help but feel the core of this should be target-independent, with TLI 
or similar hooks to actually do the target-specific bits?




Comment at: llvm/test/CodeGen/RISCV/kcfi.ll:1
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs -riscv-no-aliases < %s | 
FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs -stop-after=finalize-isel < 
%s | FileCheck %s --check-prefixes=MIR,ISEL

Please try not to introduce tests with hand-written CHECK lines, but if you 
have to, try and make them look similar in style to the auto-generated lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148385

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

[PATCH] D147989: [clang] Fix Attribute Placement

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:5041
   case DeclSpec::TST_enum:
 return 4;
   default:

Why not just always pass the full DeclSpec and handle the class case here, 
maybe with a bool to allow other users to not need to treat enum class as 
different?



Comment at: clang/lib/Sema/SemaDecl.cpp:5052-5053
+}
+  }
+  else{
+return GetDiagnosticTypeSpecifierID(DS.getTypeSpecType());

Read the style guide



Comment at: clang/lib/Sema/SemaDecl.cpp:5318
+<< AL << GetDiagnosticTypeSpecifierID(DS);
+}
   }

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147989

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

[PATCH] D148034: [clang][driver] Disable GP relaxation with RISC-V ShadowCallStack

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D148034#4262934 , @craig.topper 
wrote:

> I'm not sure I want to suggest this, but could we disable the emission of the 
> relocations that can be GP relaxed?

That would also lose the ability to do x0-relative and LUI -> C.LUI 
relaxation... though given the former only has a simm12 field that's rather 
limited in use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148034

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

[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:11390
+ResType = llvm::ScalableVectorType::get(
+llvm::Type::getIntNTy(getVMContext(), XLen), 64 / XLen);
+break;

erichkeane wrote:
> craig.topper wrote:
> > erichkeane wrote:
> > > Where is 'XLen' from here?  
> > It's a member of RISCVABIInfo. It's 64 for riscv64 triple and 32 for 
> > riscv32 triple.
> Well, the name is awful :)  I'd probably suggest a re-name and hiding it 
> behind a function call (since that way it can be done on the triple, rather 
> than an initialized variable perhaps?), but I'm not really in charge of this 
> target info.
It's not for anyone in the RISC-V space, since it is defined by the 
architecture and used pervasively (and means the X register LENgth, i.e. how 
many bits in the x0-x31 GPRs). Using anything else in a RISC-V ABI context 
would be worse from a RISC-V perspective. In a random LLVM checkout I have I 
see 1118 instances of `/xlen/i` in llvm/lib/Target/RISCV alone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088

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

[PATCH] D132819: [RISCV] Add MC support of RISCV zcmp Extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/Preprocessor/riscv-target-features.c:51
 // CHECK-NOT: __riscv_zcf {{.*$}}
+// CHECK-NOT: __riscv_zcmp
 // CHECK-NOT: __riscv_h {{.*$}}

Does this really belong in an MC patch?



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2307
+OperandMatchResultTy RISCVAsmParser::parseReglist(OperandVector ) {
+  // Rlist grammar: {ra [, s0[-sN]]} (UABI)
+  // XRlist :{x1 [, x8[-x9][, x18[-xN]]]} (UABI)

What's UABI, a term that I've only really seen in the Linux community, got to 
do with assembly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132819

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

[PATCH] D146463: [CodeGen][RISCV] Change Shadow Call Stack Register to X3

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Driver/SanitizerArgs.cpp:573
+ Kinds & SanitizerKind::ShadowCallStack)
+  << "-msmall-data-limit=0";
+}

jrtc27 wrote:
> paulkirth wrote:
> > jrtc27 wrote:
> > > Why is this an error? It may be a misguided thing to enable but it is 
> > > 100% supported to combine this. All the limit does is put things in 
> > > .sdata, but they can still be addressed just fine.
> > My understanding is that (whether they should or not) linkers are using the 
> > presence of the `.sdata` section to enable gp relaxation. 
> That is not true
Putting variables in .sdata merely makes it more likely that the linker will be 
able to relax accesses to them to use GP. Variables outside .sdata still can 
and will be relaxed if within range of whatever gets picked for 
`__global_pointer$`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146463

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

[PATCH] D146463: [CodeGen][RISCV] Change Shadow Call Stack Register to X3

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Driver/SanitizerArgs.cpp:573
+ Kinds & SanitizerKind::ShadowCallStack)
+  << "-msmall-data-limit=0";
+}

paulkirth wrote:
> jrtc27 wrote:
> > Why is this an error? It may be a misguided thing to enable but it is 100% 
> > supported to combine this. All the limit does is put things in .sdata, but 
> > they can still be addressed just fine.
> My understanding is that (whether they should or not) linkers are using the 
> presence of the `.sdata` section to enable gp relaxation. 
That is not true


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146463

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

[PATCH] D146463: [CodeGen][RISCV] Change Shadow Call Stack Register to X3

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/docs/ShadowCallStack.rst:157
+linker. This can be done with the ``--no-relax-gp`` flag in GNU ld. It may also
+be useful to compile with ``-msmall-data-limit=0``.
 

This doesn't really achieve anything other than not do one optimisation (which 
may or not matter; the grouping of small data may have a noticeable cache 
locality effect)



Comment at: clang/lib/Driver/SanitizerArgs.cpp:573
+ Kinds & SanitizerKind::ShadowCallStack)
+  << "-msmall-data-limit=0";
+}

Why is this an error? It may be a misguided thing to enable but it is 100% 
supported to combine this. All the limit does is put things in .sdata, but they 
can still be addressed just fine.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2072
 if (Args.hasArg(options::OPT_G)) {
   D.Diag(diag::warn_drv_unsupported_sdata);
 }

If you're modelling it on this warning (note, not an error) then this is crap 
too, there are legitimate reasons to use -G with -fPIC (and the -shared 
handling is even more questionable).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146463

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

[PATCH] D146463: [CodeGen][RISCV] Change Shadow Call Stack Register to X3

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

From an ABI/codegen perspective the small data limit doesn’t matter here, all 
it does is govern whether things go in .sdata or not. The linker is what 
chooses whether to use GP, which happens regardless of whether something is in 
.sdata, just things in .sdata are more likely to be picked, since GP is defined 
to be 0x800 from its start. What matters is that the linker is told not to do 
GP relaxation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146463

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

[PATCH] D146847: [NFC] Fix uninitialized member variable use in RVVEmitter::createRVVIntrinsics()

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D146847#4220721 , @schittir wrote:

> In D146847#4220697 , @jrtc27 wrote:
>
>> None of the other fields are initialised, so blindly initialising it alone 
>> to 0 here seems highly suspect. What's the actual case in which it's used 
>> whilst uninitialised?
>
> Coverity complains about SR.PolicyBitMask being uninitialized when calling 
> SemaRecords->push_back(SR)

Isn't that precisely the case 
https://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_222.htm makes legal? (Which 
I assume/hope applies equally to C++...)


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

https://reviews.llvm.org/D146847

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

[PATCH] D146847: [NFC] Fix uninitialized member variable use in RVVEmitter::createRVVIntrinsics()

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

The field should just be deleted. D126742  
added it without any uses, and I don't know what the original intent was. Maybe 
it was used before the bitfields were added and it stuck around rather than 
being GC'ed. I don't know why Coverity would care about this though when it's 
never read from, there's nothing wrong with that, unless the warning is in fact 
that the field is entirely unreferenced?


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

https://reviews.llvm.org/D146847

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

[PATCH] D146847: [NFC] Fix uninitialized member variable use in RVVEmitter::createRVVIntrinsics()

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

None of the other fields are initialised, so blindly initialising it alone to 0 
here seems highly suspect. What's the actual case in which it's used whilst 
uninitialised?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146847

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

[PATCH] D146269: MIPS: allow o32 abi with 64bit CPU and 64 abi with 32bit triple

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146269

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: flang/test/Driver/target-cpu-features-invalid.f90:13
+! CHECK-INVALID-CPU: 'supercpu' is not a recognized processor for this target 
(ignoring processor)
+! CHECK-INVALID-FEATURE: '+superspeed' is not a recognized feature for this 
target (ignoring feature)

awarzynski wrote:
> jrtc27 wrote:
> > Don't these come from the backend? Testing them here doesn't seem right...
> > Don't these come from the backend?
> 
> No. Both options are defined in Clang's Options.td:
> * [[ 
> https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/clang/include/clang/Driver/Options.td#L5251-L5253
>  | -target-feaure ]]
> * [[ 
> https://github.com/llvm/llvm-project/blob/0aac9a2875bad4f065367e4a6553fad78605f895/clang/include/clang/Driver/Options.td#L5248-L5250
>  | -target-cpu ]]
Then why do they need aarch64-registered-target? Either they're coming from the 
backend and so you need the backend, or they're coming from the frontend and so 
you don't need the backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:112-114
+  case llvm::Triple::riscv64:
+getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false);
+break;

kiranchandramohan wrote:
> jrtc27 wrote:
> > mnadeem wrote:
> > > identical code, could just do a fallthrough
> > Why is this even conditional??
> There could be other ways to do this. But it provided a way to understand the 
> list of supported architectures.
Well RISC-V needs -target-abi so its presence in the list does nothing to say 
it's properly supported. Nor do I think that's a good argument aside from that. 
You know every architecture needs to have its target features communicated, so 
making it conditional is not the right technical approach. Documenting the 
supported architectures should be done another way; perhaps in the actual 
documentation that developers/users are going to read, rather than buried in 
code?



Comment at: flang/test/Driver/code-gen-rv64.f90:7
+! RUN: %flang_fc1 -triple riscv64-unknown-linux-gnu \
+! RUN:   -target-feature +d -target-feature +c -emit-obj %s -o %t.o
+! RUN: llvm-readelf -h %t.o | FileCheck %s

Why do we need to go to an object file??? That's terrible practice in Clang 
tests, and the same should be true of Flang. Test the IR, that is sufficient, 
and decouples you from the backend.



Comment at: flang/test/Driver/target-cpu-features-invalid.f90:13
+! CHECK-INVALID-CPU: 'supercpu' is not a recognized processor for this target 
(ignoring processor)
+! CHECK-INVALID-FEATURE: '+superspeed' is not a recognized feature for this 
target (ignoring feature)

Don't these come from the backend? Testing them here doesn't seem right...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: flang/test/Driver/target-features.f90:1
+! RUN: %flang --target=riscv64-linux-gnu --target=riscv64 -c %s -### 2>&1 \
+! RUN: | FileCheck %s -check-prefix=CHECK-RV64

awarzynski wrote:
> jrtc27 wrote:
> > awarzynski wrote:
> > > What happens if the RISC-V backend is not available?
> > Clang doesn't need a backend to be available to generate IR for that 
> > architecture. I would hope Flang is the same.
> > Clang doesn't need a backend to be available to generate IR 
> 
> This test is not generating LLVM IR. It does, however, specify the target 
> which is then translated into many **LLVM**-specific flags. 
> 
> Also:
> ```
> clang --target=riscv64-linux-gnu --target=riscv64 -c file2.c
> error: unable to create target: 'No available targets are compatible with 
> triple "riscv64"'
> 1 error generated.
> ```
Because you're trying to use the backend. Pass --emit-llvm and observe it works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: flang/test/Driver/target-features.f90:1
+! RUN: %flang --target=riscv64-linux-gnu --target=riscv64 -c %s -### 2>&1 \
+! RUN: | FileCheck %s -check-prefix=CHECK-RV64

awarzynski wrote:
> What happens if the RISC-V backend is not available?
Clang doesn't need a backend to be available to generate IR for that 
architecture. I would hope Flang is the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:112-114
+  case llvm::Triple::riscv64:
+getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false);
+break;

mnadeem wrote:
> identical code, could just do a fallthrough
Why is this even conditional??


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145883

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

[PATCH] D145346: [NFC] Format printstmt

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

This isn't NFC, the output changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145346

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

[PATCH] D145214: [TSAN] add support for riscv64

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: compiler-rt/CMakeLists.txt:529
 # natively, but supports --as-needed/--no-as-needed for GNU ld compatibility.
-if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc")
+if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc|riscv")
   list(APPEND SANITIZER_COMMON_LINK_LIBS -Wl,--as-needed atomic 
-Wl,--no-as-needed)

This is only to work around a long-standing bug in GCC's atomics implementation 
for RISC-V. If you build compiler-rt with Clang you don't need this, and in 
theory future versions of GCC (possibly trunk already fixes this, I'm not sure 
of the status of it).



Comment at: compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:60
 template T func_xchg(volatile T *v, T op) {
-  T res = __sync_lock_test_and_set(v, op);
+  T res = __atomic_test_and_set(v, op);
   // __sync_lock_test_and_set does not contain full barrier.

?



Comment at: compiler-rt/lib/tsan/rtl/tsan_platform.h:971
 };
-const uptr indicator = 0x0e00ull;
+const uptr indicator = 0x0f00ull;
 const uptr ind_lsb = 1ull << LeastSignificantSetBitIndex(indicator);

?



Comment at: compiler-rt/lib/tsan/rtl/tsan_platform_linux.cpp:270-271
+#elif SANITIZER_RISCV64
+  // the bottom half of vma is allocated for userspace
+  vmaSize = vmaSize + 1;
+# if !SANITIZER_GO

Why don't other architectures need the +1?



Comment at: compiler-rt/lib/tsan/tests/CMakeLists.txt:59
+  if("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "riscv")
+list(APPEND TSAN_UNITTEST_LINK_FLAGS -latomic)
+  endif()

Same as before



Comment at: compiler-rt/test/tsan/test.h:77
+#elif defined(__riscv) && __riscv_xlen == 64
+const int kPCInc = 2;
 #else

What's this the length of? RVC is optional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145214

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/AST/ast-dump-overloaded-operators.cpp:27
 // CHECK-NEXT: | `-ParmVarDecl {{.*}}  col:19{{( imported)?}} 'E'
-// CHECK-NEXT: `-FunctionDecl {{.*}}  line:14:6{{( 
imported)?}} test 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}}  line:14:6{{( 
imported)?}} test 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} 

vabridgers wrote:
> donat.nagy wrote:
> > Why did a backtick disappear here and in many other locations? Is this 
> > somehow related to va_list handling?
> this backspace appeared in the original patch submitted by @mizvekov in 
> review https://reviews.llvm.org/D136886, discussed for the test case 
> clang/test/AST/ast-dump-overloaded-operators.cpp between @aaron.ballman and 
> @mizvekov. 
> 
> Comments from that review from @mizvekov
> 
> 
> What is happening here (and in all the other such instances) is that on the 
> import case, this declaration stops being the last one on the TU. So the 
> beginning of the line would match on |- instead of `-, but the non-import 
> case this remains the last one.
> 
> So I simply relaxed the match.
> 
Then make this abundantly clear with
```
{{\||`}}
```
or whatever the right escaping is (which, incidentally, I can't figure out how 
to do as inline code in Phabricator...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D143953: [RISCV] Accept zicsr and zifencei command line options

[Jessica Clarke via Phabricator via cfe-commits]

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

Personally happy with the concept then, seems consistent and overall helpful, 
just some nits




Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:83-86
+   "'zifencei' (ifence)">;
+def HasStdExtZifencei : Predicate<"Subtarget->hasStdExtZifencei()">,
+   AssemblerPredicate<(all_of 
FeatureStdExtZifencei),
+   "'Zifencei' (ifence)">;

s/ifence/fence.i/ (or Instruction Fence if you want words) for both these



Comment at: llvm/test/CodeGen/RISCV/attributes.ll:103-104
+; RUN: llc -mtriple=riscv64 -mattr=+zicsr %s -o - | FileCheck 
--check-prefixes=CHECK,RV64ZICSR %s
+; RUN: llc -mtriple=riscv64 -mattr=+zifencei %s -o - | FileCheck 
--check-prefixes=CHECK,RV64ZIFENCEI %s
+
 





Comment at: llvm/test/CodeGen/RISCV/attributes.ll:153-154
+; RV32ZICSR: .attribute 5, "rv32i2p0_zicsr2p0"
+; RV32ZIFENCEI: .attribute 5, "rv32i2p0_zifencei2p0"
+
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143953

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

[PATCH] D143953: [RISCV] Accept zicsr and zifencei command line options

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D143953#4124636 , @reames wrote:

> @jrtc27 Not sure if this changes your take, but I realized the variant being 
> introduced is maybe much less interesting than I'd first thought.  We 
> generally make no effort to make sure an extension was defined in the spec 
> version corresponding to our base revision.  Given that, we have a bunch of 
> cases where we allow I2.0 + some random extension.  Given that, this one 
> stops looking all that interesting.  It doesn't actually set much precedent - 
> because we already did that, a long time ago.
>
> If you agree with that framing, I'll rework the description.

Hm, do we allow M + Zmmul? If so then I guess I can get behind that view.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143953

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

[PATCH] D143953: [RISCV] Accept zicsr and zifencei command line options

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I'm fine with this approach but I think they should be filtered out of the ELF 
attributes, maybe also preprocessor macros? That is, treating them as a 
redundant no-op in the driver rather than like a true extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143953

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

[PATCH] D137309: [clang] Added Swift support for RISCV

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10557
 private:
+  DefaultABIInfo defaultInfo;
   // Size of the integer ('x') registers in bits.

Huh? This is unused.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:10601
+bool asReturnValue) const override {
+return occupiesMoreThan(CGT, scalars, /*total*/ 4);
+  }

The argument's called maxAllRegisters?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137309

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

[PATCH] D143355: [RISCV] Default to -ffixed-x18 for Fuchsia

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:16
+#include "GISel/RISCVLegalizerInfo.h"
+#include "GISel/RISCVRegisterBankInfo.h"
 #include "RISCV.h"

Unrelated change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143355

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D136886#4083762 , @vabridgers 
wrote:

> It appears to me this change https://reviews.llvm.org/D116774 is responsible 
> for the unexpected behavior. Question for @jrtc27 : do you think if we could 
> make this change consistent with https://reviews.llvm.org/D116774 that the 
> problem would be addressed? Looks like we could make consistent changes in 
> Sema.cpp and perhaps the problem would be addressed?
>
> I'm assuming that keeping __va_list in std for aarch6 and arm is a 
> requirement for the abi and not open to change?
> Best

I don't understand the question. __va_list needs to be in std on C++ 
aarch64/arm as that's part of the ABI. __va_list cannot be in std on C 
aarch64/arm as namespaces do not exist in C and having the AST say they do 
causes all manner of problems (as outlined in my prior comments on those 
revisions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

This should be readily reproducible with clang -target aarch64 on any machine, 
though? The current lit tests are just x86 ones, which isn't great coverage of 
this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Arm and AArch64 are special because they're the architectures where `va_list` 
is `std::__va_list`, not a bare `__va_list_tag` or similar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D142085: [Clang][RISCV] Add `__riscv_` prefix for all RVV intrinsics

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

(For some reason Phab won't let me comment inline from the Changeset View page, 
just does nothing when I click reply or click a line...)

Or just leave the test names alone? The __riscv_ is for namespacing the 
intrinsics, you don't need to namespace the tests when they're already in a 
RISC-V test directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142085

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

[PATCH] D139704: [clang][RISCV] Added target attributes to runtime functions

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

How does this affect LTO? LTO for RISC-V is broken for various reasons, but LTO 
isn't Clang's responsibility beyond passing on driver arguments. Is this not 
just one of many symptoms of RISC-V not having the right module-level subtarget 
under LTO, which is a real problem that needs fixing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139704

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

[PATCH] D141798: Drop the ZeroBehavior parameter from countLeadingZeros and the like (NFC)

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/include/llvm/Support/MathExtras.h:225
 ///
 /// \param ZB the behavior on an input of 0. Only ZB_Max and ZB_Undefined are
 ///   valid arguments.

A bunch of functions still have this documentation, but these are now the only 
possible values, so this seems redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141798

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

[PATCH] D141666: [RISCV] Proper support of extensions Zicsr and Zifencei

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Because they used to be part of I and then a later version of the RISC-V spec 
split them out in an incompatible way. GCC/binutils bit the bullet and split 
them out last(?) year but LLVM has yet to make that breaking change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141666

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D137517#4045315 , @fpetrogalli 
wrote:

> In D137517#4045299 , @jrtc27 wrote:
>
>> In D137517#4042875 , @fpetrogalli 
>> wrote:
>>
>>> In D137517#4042758 , @fpetrogalli 
>>> wrote:
>>>
 After submitting this, I had to revert it 
 
  because of failures like 
 https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio
>>>
>>> I have resubmitted with what I hope is the right fix (I could not reproduce 
>>> any of the failures I was seeing in buildbot, on my machine the build is 
>>> fine).
>>>
>>> The new commit is at 
>>> https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf
>>>  - in the commit message I have explained what I have changed WRT this 
>>> original patch. I have added  the
>>> tablegen target `RISCVTargetParserTableGen` in the `DEPENDS` list of 
>>> `clangDriver` and `clangBasic`. This makes sure that the `.*inc` file with 
>>> theist o the CPU is available even if `LLVMTargetParser` has not been built 
>>> yet.
>>
>> But you didn't use the proper Differential Revision tag, so the diff here 
>> didn't get updated to reflect the amended version committed :(
>
> What should I have done? Add  the `Differential Revision: 
> https://reviews.llvm.org/D137517` as the last line of the commit with the 
> rework? I wasn't aware of this for reworks.

Yes, it should have the same trailer as the original commit, otherwise it won't 
be correctly tracked by Phabricator. A "rework" isn't special, it's revert, 
reopen the revision, update the revision and land the revision again. If 
re-review isn't needed then you can skip some of the middle, but that's it. 
Though in this case I do think re-review was warranted, the new clang 
dependency seems a bit dubious and hints at the design not being quite right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D137517#4042875 , @fpetrogalli 
wrote:

> In D137517#4042758 , @fpetrogalli 
> wrote:
>
>> After submitting this, I had to revert it 
>> 
>>  because of failures like 
>> https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio
>
> I have resubmitted with what I hope is the right fix (I could not reproduce 
> any of the failures I was seeing in buildbot, on my machine the build is 
> fine).
>
> The new commit is at 
> https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf
>  - in the commit message I have explained what I have changed WRT this 
> original patch. I have added  the
> tablegen target `RISCVTargetParserTableGen` in the `DEPENDS` list of 
> `clangDriver` and `clangBasic`. This makes sure that the `.*inc` file with 
> theist o the CPU is available even if `LLVMTargetParser` has not been built 
> yet.

But you didn't use the proper Differential Revision tag, so the diff here 
didn't get updated to reflect the amended version committed :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCV.td:581
+  list f = []>
+  : ProcessorModel;
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Hm, this means that llvm/lib/Target/$TARGET/$TARGET.td needs to remain working 
(or at least mostly working, things like ISel patterns can fail to type check) 
in order for Clang to build, since we're now introducing a dependency on 
llvm/lib/Target/$TARGET in Clang where there didn't used to be (regardless of 
whether you care about that target). That's probably fine but worth pointing 
out, especially in the context of experimental targets that are supposed to be 
mostly ignorable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

[PATCH] D139302: [RISCV] Add Syntacore SCR1 CPU model

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/include/llvm/Support/RISCVTargetParser.def:22
 PROC(SIFIVE_U74, {"sifive-u74"}, FK_64BIT, {"rv64gc"})
+PROC(SCR1_BASE, {"scr1-base"}, FK_NONE, {"rv32ic"})
+PROC(SCR1_MAX, {"scr1-max"}, FK_NONE, {"rv32imc"})

Alphabetise (with the exception of INVALID)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139302

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

[PATCH] D138722: Overload all llvm.annotation intrinsics for globals argument

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/test/CodeGen/annotations-field.c:5
 
-// CHECK: private unnamed_addr constant [8 x i8] c"v_ann_{{.}}\00", section 
"llvm.metadata"
-// CHECK: private unnamed_addr constant [8 x i8] c"v_ann_{{.}}\00", section 
"llvm.metadata"
+// CHECK: private unnamed_addr[[$CONST_AS]] constant [8 x i8] 
c"v_ann_{{.}}\00", section "llvm.metadata"
+// CHECK: private unnamed_addr[[$CONST_AS]] constant [8 x i8] 
c"v_ann_{{.}}\00", section "llvm.metadata"

All these CHECK lines aren't getting consulted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138722

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

[PATCH] D139025: [NFC][RISCV] Extract utility to calculate value through MajorVersion and MinorVersion

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Should this live in RISCVISAInfo.h rather than be a Clang-specific thing given 
it's just for squeezing version info into the preprocessor? I can't think of a 
use for it outside that at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139025

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

[PATCH] D138295: [clang][TargetInfo] Use LangAS for getPointer{Width,Align}()

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2228-2230
+  Width = Target->getPointerWidth(
+  LangAS::Default); // C++ 3.9.1p11: sizeof(nullptr_t)
+  Align = Target->getPointerAlign(LangAS::Default); //   == sizeof(void*)

clang-format(?) screwed up this comment, maybe better to just put it all on one 
line before these rather than as inline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138295

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

[PATCH] D138930: [RISCV] Add macro to imply compiler availability on RISC-V Vector intrinsics version

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:196
+// Currently we support the v0.10 RISC-V V intrinsics
+unsigned Version = (0 * 100) + (10 * 1000);
+Builder.defineMacro("__riscv_v_intrinsic", Twine(Version));

Do we not have a function to do these (major, minor) -> integer encodings? I 
guess not since currently the only other instance is in the loop above. Might 
be worth factoring that out if we're going to grow more of these.



Comment at: clang/lib/Basic/Targets/RISCV.cpp:195
 Builder.defineMacro("__riscv_vector");
+// Currently we support the v0.10 RISC-V V intrinsics
+unsigned Version = (0 * 100) + (10 * 1000);

eopXD wrote:
> asb wrote:
> > Nit: comment should end with full stop.
> Sorry I just landed the commit and missed this comment. May I ask what do yo 
> mean by full stop here? Do you mean a period?
Full stop is British English for what is period in US English, yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138930

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

[PATCH] D137517: [TargetSupport] Generate the defs for RISCV CPUs using llvm-tblgen.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCV.td:509-517
+class RISCVProcessorModelPROC {
+  string Enum = enum;
+  string EnumFeatures = enum_features;
+  string DefaultMarch = default_march;
+}
+
+class RISCVProcessorModelTUNE_PROC {

Given RISCVProcessorModelTUNE_PROC is a subset of RISCVProcessorModelPROC the 
latter should inherit from the former. Also since every use is also inheriting 
from ProcessorModel why not make RISCVProcessorModelTUNE_PROC itself inherit 
from ProcessorModel?



Comment at: llvm/lib/Target/RISCV/RISCV.td:519
+
+def : ProcessorModel<"generic-rv32", NoSchedModel, [Feature32Bit]>, 
RISCVProcessorModelPROC<"GENERIC_RV32", "FK_NONE", "">;
+def : ProcessorModel<"generic-rv64", NoSchedModel, [Feature64Bit]>, 
RISCVProcessorModelPROC<"GENERIC_RV64", "FK_64BIT", "">;

This would be more natural in TableGen as `def GENERIC_RV32 : ...` and dropping 
the Enum field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

In D137350#3906126 , @craig.topper 
wrote:

> Do these need their own DecoderNameSpace?

I was going to come here to request that


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

https://reviews.llvm.org/D137350

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/docs/ReleaseNotes.rst:122
   been removed.
+* n32 was added to the RV64I datalayout string.
 

arichardson wrote:
> Without additional context I don't think this makes much sense to most 
> readers. Before looking at this patch description I would not have been and 
> to say what n is used for. 
> 
> Maybe something like "i32 has been marked as a legal integer type for RV64, 
> improving code generation for some benchmarks"?
"native integer type" not "legal integer type"; it still gets legalised during 
ISel


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116735

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

[PATCH] D136571: [RISCV] add svinval extension

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Hm, we only have two uses of Requires currently, both of which aren't really 
for any good reason as far as I can see. It'd be better to keep things uniform 
with Predicates IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136571

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

[PATCH] D135097: Remove redundant option '-menable-unsafe-fp-math'.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

It also will fail if the configured default target triple is one where the 
default program address space is non-zero, and maybe other things (do any 
targets do pre-IR name mangling for C?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135097

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

[PATCH] D135287: Disallow dereferencing of void* in C++.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

What about `__typeof__(*p)`?


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

https://reviews.llvm.org/D135287

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

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCV.td:366
+   "'Zcmt' (table jump instuctions for code-size 
reduction)", 
+   [FeatureExtZca]>; // TODO: add Zicsr as another 
dependence
+def HasStdExtZcmt : Predicate<"Subtarget->hasStdExtZcmt() && 
!Subtarget->hasStdExtC()">,

This is an odd implication, Zcmt works just fine without Zca?



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

craig.topper wrote:
> reames wrote:
> > Wait, Zcmt can't be enabled if C is?  That seems odd...
> I think Zcmt reuses encodings from the FP part of C. That's why C was split 
> into Zca, Zcf, and Zcd.
Shouldn't the conflict be enforced at a higher level? Otherwise you're silently 
making +c,+zcmt resolve to +c here.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:19
+//===--===//
+def uimm8 : Operand,
+  ImmLeaf(Imm);}]> {

Blank line



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:20
+def uimm8 : Operand,
+  ImmLeaf(Imm);}]> {
+  let ParserMatchClass = UImmAsmOperand<8>;

This should line up?



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:28
+  return false;
+return MCOp.isBareSymbolRef();
+  }];

I don't think we want this? `cm.jt sym` isn't a meaningful thing.



Comment at: llvm/lib/Target/RISCV/RISCVSystemOperands.td:383
+//===---
+// Table jump base CSR
+//===---

Capitalise (and maybe Jump Vector Table, assuming that's what jvt stands for? 
these headings come from the priv spec but that's woefully outdated for all the 
new asciidoc extensions...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133863

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

[PATCH] 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.

In D129824#3794221 , @MaskRay wrote:

> Both D54214  and this look like a surprising 
> behavior to me. Do we still have time to go back the state before D54214 
>  and make mismatching --target & -march= an 
> error?

Then there's no -m32 equivalent; that's what -march currently gives you... also 
GCC lets you do it. And -target is Clang-specific so you can't write something 
that works for both compilers.


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