I'll see if a similar change is desired for GCC too.

FWIW GNU assembler does not agree with this test case: it complains relocation out of range for baz and quux.

In D138135#4534933, @aaron.ballman wrote:

It looks like there's a test failure happening in post-commit from one of the newly added tests: https://lab.llvm.org/buildbot/#/builders/42/builds/10617 Can you investigate (and revert if that's going to take a while to resolve)?

A question: does LLD automatically supports pack-relative-relocs once a new port is added? I'm trying to implement it for LoongArch BFD linker but it seems I don't have enough skill.

The change itself LGTM.

Ooops. These corner cases are really annoying.

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 the numbers if TuneCPU is empty? If there is no differences among empty, loongarch64 and la464, seems it's better to do this until we support other uarchs, e.g. la[236]64.

Just like the TODO in D148622:

63   // TODO: Check TuneCPU and override defaults (that are for LA464) once we
64   // support optimizing for more uarchs.

Do we need to convert Xuerui's label alignment change to use the -mtune framework?

In D138135#4517125, @xen0n wrote:

In D138135#4517120, @MaskRay wrote:

In D138135#4517087, @xen0n wrote:

Friendly ping; ideally I'd like this port to get included in LLVM 17, which will vastly simplify things ecosystem-wide, such as ClangBuiltLinux testing, Gentoo LLVM stagebuilding, and possibly many more large projects depending on lld.

This is on my radar and I will take a look again soon. There are also random other LoongArch things I need to take a look and I did. This is totally fine to be cherry picked into the release/17.x branch even after it is created.
I am sorry that I did not respond in time for the initial period of the patch (when it changed from the "WIP" state to the normal in-review state), but I think the request after 3 days may be a bit too quick.
Honestly the linker is probably I care about the most and I don't want to lower the bar. (There are other things that I am concerned with as well, but I may not have too much energy to prevent something less appropriate happen; but if I notice, I'll make a comment).

Thanks for the clarification. I didn't know if the release branch cherry-pick could be done in this case so I was a little anxious, sorry for the less-than-a-week ping.

I fully understand your passion and care for the linker and I agree with not lowering the bar; I'll do my best to improve the code together.

In D153865#4452321, @SixWeining wrote:

Interesting. Does cpucfg have the same issue?

Ah, in the C standard "reading a volatile variable" is indeed a side effect. I must have been thinking about another definition of "side effect".

But it's puzzling. Is reading a CSR really a side effect?

LGTM.

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.

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 cases. For example:

struct { struct{}; int i; }; // in this case, the empty struct is not ignored.
struct { struct{}; float f; }; // ignored.

This is the same behavior as RISC-V. While attempting to pass a struct through FPRs, the empty field is ignored. But if passing through FPR does not work and it's passed through GPRs, the empty fields are not ignored:

Yes, but our psABI still differs from RISC-V in the description of parameter passing, and it would be better to have consistent behavior regardless of whether there are floating points or not.

In D151298#4367496, @xry111 wrote:

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, a will be passed with a1 register.

I mean now GCC and Clang have the same behavior, so it's easier to just document the behavior in our psABI doc instead of making both Clang and GCC rigidly following the interpretation of psABI (which is currently unclear about zero-sized fields now) anyway.

And the current behavior of GCC and Clang treating a class containing two floating-point members and some empty fields is same as RISC-V, so it's highly unlikely we are violating the C++ standard or IA64 C++ ABI (or the entire RISC-V ecosystem would be violating them). The only issue is our psABI is unclear about empty fields, and the easiest way to solve the issue is revising the psABI (maybe, just "borrowing" some paragraphs from RISC-V psABI if there is no copyright issue).

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 cases. For example:

struct { struct{}; int i; }; // in this case, the empty struct is not ignored.
struct { struct{}; float f; }; // ignored.

This is the same behavior as RISC-V. While attempting to pass a struct through FPRs, the empty field is ignored. But if passing through FPR does not work and it's passed through GPRs, the empty fields are not ignored:

https://godbolt.org/z/T1PKoxbYM

Whether to ignore empty structures or not can affect the testing of gdb. @seehearfeel knows more details.

I guess we should be able to fix it for GDB because they must have fixed it for RISC-V already.

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, a will be passed with a1 register.

I mean now GCC and Clang have the same behavior, so it's easier to just document the behavior in our psABI doc instead of making both Clang and GCC rigidly following the interpretation of psABI (which is currently unclear about zero-sized fields now) anyway.

And the current behavior of GCC and Clang treating a class containing two floating-point members and some empty fields is same as RISC-V, so it's highly unlikely we are violating the C++ standard or IA64 C++ ABI (or the entire RISC-V ecosystem would be violating them). The only issue is our psABI is unclear about empty fields, and the easiest way to solve the issue is revising the psABI (maybe, just "borrowing" some paragraphs from RISC-V psABI if there is no copyright issue).

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 cases. For example:

struct { struct{}; int i; }; // in this case, the empty struct is not ignored.
struct { struct{}; float f; }; // ignored.

In D151298#4367349, @wangleiat wrote:

In D151298#4367225, @xry111 wrote:

In D151298#4367215, @wangleiat wrote:

In D151298#4367163, @xry111 wrote:

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++ standard only treats struct {} as size 1 for the semantics of pointer comparison. While there is no pointers to registers, ignoring it in the register calling convention will make no harm.

And AFAIK it will be an undefined behavior attempting to (mis)use the padding space of/after the empty struct to pass any information.

Our current modifications are closer to the description of Itanium C++ ABI, and we try to keep it consistent with the description of the calling convention under reasonable premise.

Hmm, could you provide a link to the section saying this in the Itanium C++ ABI?

http://itanium-cxx-abi.github.io/cxx-abi/abi.html#empty-parameters
http://itanium-cxx-abi.github.io/cxx-abi/abi.html#emptty-return-values

I see it has some words about passing or returning an empty class, but there seems no words about passing a class containing an empty class.

It's possible that my understanding is incorrect. There is indeed no place to see how an empty class is passed, just like there is no documentation on how to pass class A { class B { char c;};};.

In D151298#4367215, @wangleiat wrote:

In D151298#4367163, @xry111 wrote:

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++ standard only treats struct {} as size 1 for the semantics of pointer comparison. While there is no pointers to registers, ignoring it in the register calling convention will make no harm.

And AFAIK it will be an undefined behavior attempting to (mis)use the padding space of/after the empty struct to pass any information.

Our current modifications are closer to the description of Itanium C++ ABI, and we try to keep it consistent with the description of the calling convention under reasonable premise.

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?

Blocking this as it's a deliberate decision made in D132285.

I guess the most easy way is adding -flifetime-dse{,=1,=2} and -fno-lifetime-dse as no-op into Clang (and we can really implement -flifetime-dse{,=1,=2} in the future). But not sure if it's the "correct" thing to do.

Hmm, I'd tested the change before creating this but it seems those tests are "UNSUPPORTED" on my system :(.

In D150505#4342005, @uabelho wrote:

Hello,

The following four testcases fail for me with this patch:

Failed Tests (4):
  Clangd :: check-fail.test
  Clangd :: check-lines.test
  Clangd :: check.test
  Clangd :: path-mappings.test

Seems like some bots fail as well, e.g. https://lab.llvm.org/buildbot/#/builders/121/builds/30482

In D150505#4342005, @uabelho wrote:

Hello,

The following four testcases fail for me with this patch:

Failed Tests (4):
  Clangd :: check-fail.test
  Clangd :: check-lines.test
  Clangd :: check.test
  Clangd :: path-mappings.test

Seems like some bots fail as well, e.g. https://lab.llvm.org/buildbot/#/builders/121/builds/30482

In D150505#4341931, @jhuber6 wrote:

In D150505#4341921, @xry111 wrote:

It's already fixed by rG626849c71e85d546a004cc91866beab610222194. See above.

I'm fairly certain that patch only fixes the failing test and not the underlying problem. I don't know if there's a good way to work around this.

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

Added a "depends on" line in the commit msg, so not to puzzle people to wonder why this is landed again w/o change.

With rG626849c71e85d546a004cc91866beab610222194 landed, this should be safe to land now.

Closing as https://reviews.llvm.org/rG626849c71e85d546a004cc91866beab610222194 is already landed and we just need to reopen D150505.

Revised changeset at D150524.

Wow. The problem is clang-tidy using the compile_commands.json file in the build directory as a database for options (because the trivially-destructible.cpp.tmp.cpp file is in the build directory). As trivially-destructible.cpp.tmp.cpp is not really in compile_commands.json, it uses the options for tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar-driver.cpp.o which is compiled by GCC with -fno-lifetime-dse...

In D150505#4340437, @thesamesam wrote:

I can only reproduce it with a GCC-built LLVM.

Anyway, given LLVM_COMPILER_IS_GCC_COMPATIBLE is true for Clang (see llvm/cmake/modules/DetermineGCCCompatible.cmake), it seems easiest for us to just check CMAKE_COMPILER_IS_GNUCXX instead (given Clang doesn't acknowledge -fno-lifetime-dse at all).

I'll test that now.

I'm also investigating.

Thanks @nikic for grammar fix.

Update comment and commit message to fix grammar errors according to suggestion from @nikic.

In D150196#4329730, @SixWeining wrote:

In D150196#4329518, @xry111 wrote:

Hmm, I think it's a kernel issue and should be fixed by using -mabi=lp64s instead of -msoft-float for this specific file...

Hmm, I think it's a kernel issue and should be fixed by using -mabi=lp64s instead of -msoft-float for this specific file...

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 supporting models, instead of for the generic loongarch64 model too. This is because although all server- and desktop-class LoongArch models have UAL, the embedded-class (Loongson-1 and Loongson-2 series' older models) doesn't, and some of them e.g. Loongson 2K1000LA are readily available on the market so they're arguably relevant. We don't want to generate misaligned memory accesses for those systems only to fall back to much slower emulation later.

If so, CPUs that support UAL will not benefit from this feature in default build (i.e. without -march or -mcpu or -mtune being specified).

Does generic model mean the lowest model or the most popular model?

Lulu said a benchmark should be performed for such a change (via gcc-patches) and I agree, so please hold the change until a benchmark is done.

In D148622#4277187, @xen0n wrote:

In D148622#4277173, @xry111 wrote:

Hmm, it seems to me setMaxBytesForAlignment sets "the maximum amount of padding allowed for aligning the BB", not the max loop size. Or am I missing something?

I simply copied from AArch64 for that part. Maybe I should mention this in the commit message...

Hmm, it seems to me setMaxBytesForAlignment sets "the maximum amount of padding allowed for aligning the BB", not the max loop size. Or am I missing something?

In D136659#4233990, @nikic wrote:

In D136659#4233968, @xry111 wrote:

We observed a crash in Thunderbird built with Rustc using LLVM 16.0.0 (happens with Rustc 1.68.x built with a system LLVM 16, and Rustc nightly 2023-03-25). See https://bugzilla.mozilla.org/show_bug.cgi?id=1824544.

Bisect suggests this change is the first bad commit.

Any pointer for diagnose?

Probably the same issue as https://github.com/llvm/llvm-project/issues/58776. It needs a smaller reproducer than "build all of firefox".

We observed a crash in Thunderbird built with Rustc using LLVM 16.0.0 (happens with Rustc 1.68.x built with a system LLVM 16, and Rustc nightly 2023-03-25). See https://bugzilla.mozilla.org/show_bug.cgi?id=1824544.

In D145494#4178090, @nikic wrote:

Do you have commit access, or do you need somebody to land this for you? If so, could you please share the Name <email> to use for the commit?

In D143880#4122602, @xen0n wrote:

In D143880#4121913, @xen0n wrote:

@xry111: BTW your GCC implementation of this pattern looks for *arithmetic* right shifts; do you mean logical instead? Because obviously the higher bits would be full of ones after shifting if rj happen to have its MSB set. The sign extension shall happen *after* the pick.

My bad, I'm not familiar with GCC RTL patterns so I've mistakenly thought "lshiftrt could be some 'left' shift, so ashift must be an arithmetic right shift". Actually your pattern is correct as you have perfect test coverage. Please ignore my comment...

FWIW, BFD linker handles R_LARCH_PCALA_HI20 by creating a PLT for every R_LARCH_PCALA_HI20 against an undefined symbol. This can be really annoying if someone happens to make a simple typo in a symbol name (causing an runtime crash which is difficult to be diagnosed). And I guess using such an approach in LLD will annoy LLD maintainers.

By the way, using PCALA_LO12 like this means lld will need to peek at the instruction and see if it's a jirl to handle the PCALA_LO12 relocation properly. I guess we'll need to investigate this issue a little. I hope it won't cause too much trouble.

LGTM though I really dislike depending on some undocumented behavior of PCALA_LO12.

And maybe this need to be sequenced after D138481?

Mostly LGTM, I was considering this optimization just before seeing this diff.

LGTM, thanks for fixing my stupid error!

In D137532#3924134, @xen0n wrote:

In D137532#3924037, @gonglingqin wrote:

In D137532#3923952, @xen0n wrote:

I'm curious what usage case motivated the change? Not to say it's unnecessary but it's better to have people know a bit more background behind this work.

This interface is required by the code in https://github.com/loongson/linux/blob/master/arch/loongarch/include/asm/percpu.h#L12-L18. This patch can avoid crashes when compiling the kernel with llvm in the future.

Very nice to know.

BTW, are the normal GPRs already supported like this too (things like global register long foo __asm__("$a0")), even without this change, or are such usages invalid in the first place? Memory is failing me.

If I read the code correctly, calling __builtin_frame_address with a depth > 0 is only used in tests. Can we disable those tests if __builtin_frame_address cannot handle depth > 0?

GCC documentation claims:

Is it possible to implement __builtin_frame_address for LoongArch then?

In D137396#3909664, @SixWeining wrote:

LGTM.
But I see the comments in this file:

// Kernel ABI...
//  syscall number is passed in a7
//  (http://man7.org/linux/man-pages/man2/syscall.2.html) results are return in
//  a0 and a1 (http://man7.org/linux/man-pages/man2/syscall.2.html) arguments
//  are passed in: a0-a7 (confirmed by inspecting glibc sources).

// results are return in a0 and a1

Is this correct?

Thanks for fixing another instance of stupid programming errors made by me!

No, a1-a7 are saved by syscall.

Hmm, BFD linker does not support R_LARCH_GOT_PC_{HI12,LO20} with a non-zero addend:

Straightforward enough to me.

In D137231#3904351, @XiaodongLoong wrote:

What is the kernel version that support this flags? Maybe give a link on Github?

Enable ASAN vfork test for loongarch64.

In D137160#3901074, @SixWeining wrote:

So this can make compiler-rt/test/asan/TestCases/Linux/vfork.cpp pass on loongarch64?

Thanks but how to apply a patch to https://gcc.gnu.org/git/?p=gcc.git 's libasan is a step I missed...

In D137160#3899759, @MaskRay wrote:

If you can write down how to test this somewhere, then I can verify such changes if needed in the future.

Hmm, should I modify the diff to make git clang-format happy?

@tangyouling I guess this will fix some test failures you've noticed.