[PATCH] D156116: [Clang][LoongArch] Fix ABI handling of empty structs in C++ to match GCC behaviour

xry111 accepted this revision. xry111 added a comment. This revision is now accepted and ready to land. The change itself LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156116/new/ https://reviews.llvm.org/D156116

[PATCH] D156116: [Clang][LoongArch] Fix ABI handling of empty structs in C++ to match GCC behaviour

xry111 added a comment. Ooops. These corner cases are really annoying. I think I should try `make check-gcc check-g++ RUNTESTFLAGS='ALT_CC_UNDER_TEST=clang ALT_CXX_UNDER_TEST=clang++ compat.exp'` in GCC test suite... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D155824: [LoongArch] Support -march=native and -mtune=

xry111 added a comment. In D155824#4521047 , @SixWeining wrote: > In D155824#4518597 , @xry111 wrote: > >> Do we need to convert Xuerui's label alignment change to use the -mtune >> framework? > > How to set

[PATCH] D155824: [LoongArch] Support -march=native and -mtune=

xry111 added a comment. Do we need to convert Xuerui's label alignment change to use the -mtune framework? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155824/new/ https://reviews.llvm.org/D155824 ___

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. If you are really determined to do this, then OK. I'm in a very bad mood and I don't want to spend my mental strength on debating (esp. on a corner case unlikely to affect "real" code) anymore. But remember to add a entry in GCC 14 changes.html, and test this thing:

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. In D151298#4367620 , @wangleiat wrote: If we want to ignore empty structures, it is not enough to only modify psABI, because the current implementations of gcc and clang do not ignore empty structures in all

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. In D151298#4367496 , @xry111 wrote: > In D151298#4367458 , @wangleiat > wrote: > >>> I think the paragraph means: >>> >>> class Empty {}; >>> int test(Empty empty, int a); >>> >>>

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. In D151298#4367458 , @wangleiat wrote: >> I think the paragraph means: >> >> class Empty {}; >> int test(Empty empty, int a); >> >> Then we should put `a` into `a1`, not `a0`. And we are indeed doing so. > > yes. with this

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. In D151298#4367349 , @wangleiat wrote: > In D151298#4367225 , @xry111 wrote: > >> In D151298#4367215 , @wangleiat >> wrote: >> >>> In

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. In D151298#4367215 , @wangleiat wrote: > In D151298#4367163 , @xry111 wrote: > >> Blocking this as it's a deliberate decision made in D132285 >> . >> >>

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 added a comment. Note that we are using the "de-facto" ABI throwing away this empty struct since the first release of GCC supporting LoongArch, and also Clang. So is there an imperative reason we must change it? And IIUC we've agreed to revise the ABI since Apr 2022 for this, but

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

xry111 requested changes to this revision. xry111 added a comment. This revision now requires changes to proceed. Blocking this as it's a deliberate decision made in D132285 . Is there any imperative reason we must really pass the empty struct? The C++

[PATCH] D150582: [clangd] Fix test failure when it's built with compiler flags unknown by clang

xry111 added a comment. Hmm, I'd tested the change before creating this but it seems those tests are "UNSUPPORTED" on my system :(. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150582/new/ https://reviews.llvm.org/D150582

[PATCH] D150582: [clangd] Fix test failure when it's built with compiler flags unknown by clang

xry111 created this revision. xry111 added reviewers: thesamesam, MaskRay, uabelho. Herald added subscribers: kadircet, arphaman. Herald added a project: All. xry111 requested review of this revision. Herald added subscribers: cfe-commits, ilya-biryukov. Herald added a project: clang-tools-extra.

[PATCH] D149946: [LoongArch] Define `ual` feature and override `allowsMisalignedMemoryAccesses`

xry111 added a comment. OTOH if the Linux kernel is expected to emulate UAL, -march=generic should be //allowed// to generate unaligned access instructions for Linux targets. Frankly I prefer this personally, But I'm not sure if it will be a good practice in general... Repository: rG LLVM

[PATCH] D150524: [cmake] Disable GCC lifetime DSE

xry111 added a comment. Closing as https://reviews.llvm.org/rG626849c71e85d546a004cc91866beab610222194 is already landed and we just need to reopen D150505 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150524/new/ https://reviews.llvm.org/D150524

[PATCH] D150524: [cmake] Disable GCC lifetime DSE

xry111 created this revision. Herald added subscribers: ekilmer, PiotrZSL, carlosgalvezp. Herald added a project: All. xry111 requested review of this revision. Herald added projects: LLVM, clang-tools-extra. Herald added subscribers: cfe-commits, llvm-commits. LLVM data structures like

[PATCH] D149946: [LoongArch] Define `ual` feature and override `allowsMisalignedMemoryAccesses`

xry111 added a comment. In D149946#4324877 , @SixWeining wrote: > In D149946#4324803 , @xen0n wrote: > >> From a LoongArch developer's perspective, it may be better to only enable >> UAL for LA464 and other

[PATCH] D142688: [Clang][Driver] Handle LoongArch multiarch tuples

xry111 added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.cpp:66 + default: +return IsLA32 ? "ilp32d" : "lp64d"; + } SixWeining wrote: > Better to be `f`? (Probably most 32-bit hardwares do not support > double-float? But I'm

[PATCH] D138489: [tsan] Add tsan support for loongarch64

xry111 added inline comments. Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:1543 +"r"(__fn), "r"(__arg), "r"(nr_clone), "i"(__NR_exit) + : "memory"); + return res; xry111 wrote: > SixWeining wrote: > > tangyouling wrote: > > >

[PATCH] D138489: [tsan] Add tsan support for loongarch64

xry111 added inline comments. Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:1543 +"r"(__fn), "r"(__arg), "r"(nr_clone), "i"(__NR_exit) + : "memory"); + return res; SixWeining wrote: > tangyouling wrote: > > SixWeining wrote: > >

[PATCH] D138489: [tsan] Add tsan support for loongarch64

xry111 added inline comments. Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux.h:80 void internal_sigdelset(__sanitizer_sigset_t *set, int signum); -#if defined(__x86_64__) || defined(__mips__) || defined(__aarch64__) || \ -defined(__powerpc64__) ||

[PATCH] D136146: [Clang][LoongArch] Handle -march/-m{single,double,soft}-float/-mfpu options

xry111 added inline comments. Comment at: clang/lib/Driver/ToolChains/Arch/LoongArch.cpp:44 + // Select abi based on -mfpu=xx. + if (const Arg *A = Args.getLastArg(options::OPT_mfpu_EQ)) { MaskRay wrote: > It's better to stick with just one canonical

[PATCH] D136436: [Clang][LoongArch] Add register alias handling without `$` prefix

xry111 added a comment. In D136436#3898398 , @xry111 wrote: > In D136436#3898392 , @tangyouling > wrote: > >> Is it acceptable to add the `non-prefix $`? if not, an alternative fix for >> the build failure is

[PATCH] D136436: [Clang][LoongArch] Add register alias handling without `$` prefix

xry111 added a comment. In D136436#3898392 , @tangyouling wrote: > Is it acceptable to add the `non-prefix $`? if not, an alternative fix for > the build failure is to add the `prefix $` in > sanitizer_syscall_linux_loongarch64.inc I wrote

[PATCH] D136413: [Clang][LoongArch] Define more LoongArch specific built-in macros

xry111 added a comment. Do we support `--target=loongarch64 -mabi=ilp32d` or `-mfpu=64 -mabi=lp64s` combinations now? If true I think we'll need test cases for such combinations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136413/new/

[PATCH] D132285: [LoongArch] Implement ABI lowering

xry111 added a comment. In D132285#3737855 , @SixWeining wrote: > Ignore zero-width bit fields and add a test. Thanks! There is a GCC test (https://gcc.gnu.org/r12-7951) which can be used to test if there is any ABI incompatibility between GCC and

[PATCH] D132285: [LoongArch] Implement ABI lowering

xry111 added a comment. Are cases like struct x { double a; __int128 : 0; double b;}; double f(struct x x) { return x.a + x.b; } handled properly? AFAIK RISC-V clang currently does not handle it correctly: https://godbolt.org/z/rvM99zbqc (GCC handles it properly)

[PATCH] D130255: [Clang][LoongArch] Add initial LoongArch target and driver support

xry111 added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2226 + static const char *const LoongArch64LibDirs[] = {"/lib64", "/lib"}; + static const char *const LoongArch64Triples[] = { SixWeining wrote: > xry111 wrote: > > SixWeining

[PATCH] D130255: [Clang][LoongArch] Add initial LoongArch target and driver support

xry111 added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2226 + static const char *const LoongArch64LibDirs[] = {"/lib64", "/lib"}; + static const char *const LoongArch64Triples[] = { SixWeining wrote: > MaskRay wrote: > > I don't know