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

[Xi Ruoyao via Phabricator via cfe-commits]

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

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D156116/new/

https://reviews.llvm.org/D156116

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

In GCC I added the numbers for "loongarch64" and "la464" although they are the 
same, and GCC does not have "empty".  But yes we can do things later here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155824

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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:

struct Empty {};

struct Something : Empty
{

  double a, b;

};

If we are not careful enough we may introduce a ABI mismatch between -
std=c++14 and -std=c++17 here.  See https://gcc.gnu.org/PR94383.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

But it's easier to just modify the text of the ABI doc, in order to avoid an 
ABI incompatibility between different GCC of Clang versions (as GCC and Clang 
are only C++ compilers supporting LoongArch now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

@seahearfeel: 
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=9f0272f8548164b024ff9fd151686b2b904a5d59


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

xry111 added a comment.

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;};};`.

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.

>> And for our own psABI, it's easier to simply reword it (making it similar to 
>> the RISC-V psABI about passing args with FPRs) instead of modifying both 
>> Clang and GCC (causing the code of Clang and GCC more nasty, and both 
>> compilers slower, and we'll need to add a -Wpsabi warning at least in GCC 
>> too).  And it already needs a reword considering empty arrays and zero-width 
>> bit-fields anyway.
>
> I'm sorry, I couldn't quite understand what you meant.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

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.

And for our own psABI, it's easier to simply reword it (making it similar to 
the RISC-V psABI about passing args with FPRs) instead of modifying both Clang 
and GCC (causing the code of Clang and GCC more nasty, and both compilers 
slower, and we'll need to add a -Wpsabi warning at least in GCC too).  And it 
already needs a reword considering empty arrays and zero-width bit-fields 
anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 
unfortunately I've never got enough English skill to do the revision myself :(.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151298

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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.

If LLVM is built with a compiler other than clang, the `compile_commands.json`
file may contain compiler flags unknown by clang.  When a clangd test is copied
into the build directory and checked, clangd will pick the unknown flag from
the file and cause a test failure.  Create an empty `compile_commands.json` in
the test directory nested in the build directory to override it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150582

Files:
  clang-tools-extra/clangd/test/CMakeLists.txt
  clang-tools-extra/clangd/test/compile_commands.json


Index: clang-tools-extra/clangd/test/compile_commands.json
===
--- /dev/null
+++ clang-tools-extra/clangd/test/compile_commands.json
@@ -0,0 +1 @@
+[]
Index: clang-tools-extra/clangd/test/CMakeLists.txt
===
--- clang-tools-extra/clangd/test/CMakeLists.txt
+++ clang-tools-extra/clangd/test/CMakeLists.txt
@@ -28,6 +28,16 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.cfg.py
   )
 
+# Copy an empty compile_commands.json to override the compile_commands.json
+# in the top level build directory.  Or if a clangd test involves creating a
+# temporary source file in the build directory and run clangd to check it,
+# it can pick up unrecognizable command options when LLVM is built with
+# another compiler or a different version of Clang.
+configure_file(
+  ${CMAKE_CURRENT_SOURCE_DIR}/compile_commands.json
+  ${CMAKE_CURRENT_BINARY_DIR}/compile_commands.json
+)
+
 add_lit_testsuite(check-clangd "Running the Clangd regression tests"
   # clangd doesn't put unittest configs in test/unit like every other project.
   # Because of that, this needs to pass two folders here, while every other


Index: clang-tools-extra/clangd/test/compile_commands.json
===
--- /dev/null
+++ clang-tools-extra/clangd/test/compile_commands.json
@@ -0,0 +1 @@
+[]
Index: clang-tools-extra/clangd/test/CMakeLists.txt
===
--- clang-tools-extra/clangd/test/CMakeLists.txt
+++ clang-tools-extra/clangd/test/CMakeLists.txt
@@ -28,6 +28,16 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.cfg.py
   )
 
+# Copy an empty compile_commands.json to override the compile_commands.json
+# in the top level build directory.  Or if a clangd test involves creating a
+# temporary source file in the build directory and run clangd to check it,
+# it can pick up unrecognizable command options when LLVM is built with
+# another compiler or a different version of Clang.
+configure_file(
+  ${CMAKE_CURRENT_SOURCE_DIR}/compile_commands.json
+  ${CMAKE_CURRENT_BINARY_DIR}/compile_commands.json
+)
+
 add_lit_testsuite(check-clangd "Running the Clangd regression tests"
   # clangd doesn't put unittest configs in test/unit like every other project.
   # Because of that, this needs to pass two folders here, while every other
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 Github Monorepo

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

https://reviews.llvm.org/D149946

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 llvm::User and llvm::MDNode rely on
the value of object storage persisting beyond the lifetime of the
object (#24952). This is not standard compliant and causes a runtime
crash if LLVM is built with GCC and LTO enabled (#57740). Until
these issues are fixed, we need to disable dead store eliminations
eliminations based on object lifetime.

The previous version (D150505 ) also 
exploited an issue in the
clang-tidy test suite.  The test `trivially-destructible.cpp` is filtered
into a temporary file, and when clang-tidy operates on the file, the
`compile_commands.json` file will be read and the compiler options
in it will be used.  If LLVM is not built with Clang, some of the options
may be unsupported, causing a test failure.  Fix it by adding "-p %s"
into the test command to force clang-tidy to search the compiler
option DB file in the source directory, where there is none.

Bug: https://github.com/llvm/llvm-project/issues/24952
Bug: https://github.com/llvm/llvm-project/issues/57740
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106943


https://reviews.llvm.org/D150524

Files:
  
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
  llvm/cmake/modules/HandleLLVMOptions.cmake


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
@@ -1,7 +1,14 @@
 // RUN: %check_clang_tidy %s performance-trivially-destructible %t
 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -fix
-// RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' 
-warnings-as-errors='-*,performance-trivially-destructible'
+// RUN: clang-tidy -p %S %t.cpp 
-checks='-*,performance-trivially-destructible' -fix
+// RUN: clang-tidy -p %S %t.cpp 
-checks='-*,performance-trivially-destructible' 
-warnings-as-errors='-*,performance-trivially-destructible'
+
+// The "-p %S" in the clang-tidy command lines above is used for overriding
+// the detection of the `compile_commands.json` file.  As %t.cpp is in the
+// build directory, by default the `compile_commands.json` file created by
+// cmake for LLVM itself will be used.  But the file may contains options
+// not recognized by Clang if the LLVM project is built with another
+// compiler.
 
 struct TriviallyDestructible1 {
   int a;
Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -594,6 +594,16 @@
   add_flag_if_supported("-Werror=unguarded-availability-new" 
WERROR_UNGUARDED_AVAILABILITY_NEW)
 endif( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
 
+if ( LLVM_COMPILER_IS_GCC_COMPATIBLE )
+  # LLVM data structures like llvm::User and llvm::MDNode rely on
+  # the value of object storage persisting beyond the lifetime of the
+  # object (#24952).  This is not standard compliant and causes a runtime
+  # crash if LLVM is built with GCC and LTO enabled (#57740).  Until
+  # these bugs are fixed, we need to disable dead store eliminations
+  # based on object lifetime.
+  add_flag_if_supported("-fno-lifetime-dse" CMAKE_CXX_FLAGS)
+endif ( LLVM_COMPILER_IS_GCC_COMPATIBLE )
+
 # Modules enablement for GCC-compatible compilers:
 if ( LLVM_COMPILER_IS_GCC_COMPATIBLE AND LLVM_ENABLE_MODULES )
   set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})


Index: clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance/trivially-destructible.cpp
@@ -1,7 +1,14 @@
 // RUN: %check_clang_tidy %s performance-trivially-destructible %t
 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -fix
-// RUN: clang-tidy %t.cpp -checks='-*,performance-trivially-destructible' -warnings-as-errors='-*,performance-trivially-destructible'
+// RUN: clang-tidy -p %S %t.cpp -checks='-*,performance-trivially-destructible' -fix
+// RUN: clang-tidy -p %S %t.cpp -checks='-*,performance-trivially-destructible' -warnings-as-errors='-*,performance-trivially-destructible'
+
+// The "-p %S" in the clang-tidy command lines above is used for overriding
+// 

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

[Xi Ruoyao via Phabricator via cfe-commits]

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

Technically `generic` should mean the `lowest`.  But as a desktop user, frankly 
I don't want to pay the extra cost for 2K models.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149946

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 not sure about this.)
The ISA manual says 64-bit float support is not limited by 32-bit basic ISA 
support (except the moving instructions accessing 64-bit GPR).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142688

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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:
> > > SixWeining wrote:
> > > > Shall we list $t0-$t8 here? Ref D137396.
> > > > Shall we list $t0-$t8 here? Ref D137396.
> > > 
> > > I will add the missing $t0-$t8.
> > As this inline asm is almost at the end of the function, it's sure `$t*` 
> > would not be used any more. So I think we could not add them to the clobber 
> > list. Just keep current approach.
> It's hard to tell: the compiler may decide to inline this function of perform 
> some inter-procedural analysis.  So IMO we should add t0-t8 here.
> It's hard to tell: the compiler may decide to inline this function of perform 
> some inter-procedural analysis.  So IMO we should add t0-t8 here.

"or perform", not "of perform".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138489

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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:
> > > Shall we list $t0-$t8 here? Ref D137396.
> > > Shall we list $t0-$t8 here? Ref D137396.
> > 
> > I will add the missing $t0-$t8.
> As this inline asm is almost at the end of the function, it's sure `$t*` 
> would not be used any more. So I think we could not add them to the clobber 
> list. Just keep current approach.
It's hard to tell: the compiler may decide to inline this function of perform 
some inter-procedural analysis.  So IMO we should add t0-t8 here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138489

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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__) || defined(__s390__) || defined(__i386__) || \
-defined(__arm__) || SANITIZER_RISCV64
+#if defined(__x86_64__) || defined(__mips__) || defined(__aarch64__) || \
+defined(__powerpc64__) || defined(__s390__) || defined(__i386__) || \

SixWeining wrote:
> May be it's better to keep original indention. Otherwise the paired `#endif` 
> doesn't look good.
The problem is `git clang-format` sometimes insist you to change the 
indentation...



Comment at: compiler-rt/test/sanitizer_common/print_address.h:12
+defined(__s390x__) || (defined(__riscv) && __riscv_xlen == 64) ||  
\
+defined(__loongarch__)
 // On FreeBSD, the %p conversion specifier works as 0x%x and thus does not

SixWeining wrote:
> Should be __loongarch64?
`__loongarch64` is deprecated, use `__loongarch_lp64` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138489

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 spelling. Is there time to 
> remove support for one set of options? When `-mfpu=64 -msoft-float` is 
> specified, is a -Wunused-command-line-argument warning expected?
According to the doc, the semantics of -mfpu=64 and -mdouble-float are not 
exactly same.  `-mfpu=64 -mabi=lp64s` will allow the compiler to generate 
64-bit floating-point instructions but keep LP64S calling convention.  
`-mdouble-float -mabi=lp64s` will be same as `-mdouble-float -mabi=lp64d` (with 
a warning emitted, maybe).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136146

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 to add the `prefix $` in 
>> sanitizer_syscall_linux_loongarch64.inc
>
> I wrote `sanitizer_syscall_linux_loongarch64.inc` and tested it with GCC.  
> GCC supports register variable definition like `register u64 a0 asm("a0");` 
> but does not support `addi.d a0, a0, a1` (because in GCC such a line in 
> inline assembly is passed to GNU as directly).
>
> Is it possible to "emulate" the behavior in Clang?

I don't have any objection to add `$` into 
`sanitizer_syscall_linux_loongarch64.inc` though, GCC supports both `register 
u64 a0 asm("a0");` and `register u64 a0 asm("$a0");`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136436

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 `sanitizer_syscall_linux_loongarch64.inc` and tested it with GCC.  GCC 
supports register variable definition like `register u64 a0 asm("a0");` but 
does not support `addi.d a0, a0, a1` (because in GCC such a line in inline 
assembly is passed to GNU as directly).

Is it possible to "emulate" the behavior in Clang?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136436

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D136413

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

[PATCH] D132285: [LoongArch] Implement ABI lowering

[Xi Ruoyao via Phabricator via cfe-commits]

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 Clang, but I guess 
it's not possible to run it at the early development stage of Clang.  Maybe we 
can run it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132285

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

[PATCH] D132285: [LoongArch] Implement ABI lowering

[Xi Ruoyao via Phabricator via cfe-commits]

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)
https://godbolt.org/z/sWY5vs5ce (Clang does not handle it properly)

Note that we rectified the ABI to match the behavior of RISC-V GCC deliberately 
(https://gcc.gnu.org/r12-8294) but I didn't rewrite the document because the 
behavior is not easy to describe with my poor English.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132285

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 wrote:
> > > MaskRay wrote:
> > > > I don't know which of /lib64, /lib has been used. For purity, I'd hope 
> > > > that we just have /lib, no multilib style /lib64
> > > I also don't know the actual usage of /lib64 but I just tried and it 
> > > works fine if I remove /lib64.
> > I don't like `lib64` too.  But for LoongArch LP64D, the path to ELF 
> > interpreter is hard coded `/lib64/ld-linux-loongarch-lp64d.so.1` and it 
> > seems too late to change it.  And LoongArch GCC installs libstdc++ etc. for 
> > LP64D into $PREFIX/lib64 by default (like x86_64).
> > 
> > As a distro (LFS) maintainer: we are already hacking GCC code to get rid of 
> > `/usr/lib64`.
> Thanks for the quick reply. So I should keep the /lib64 here?
I think you should keep it.  A multilib distro may have `/usr/lib64`, 
`/usr/lib32`, and `/usr/lib32sf` (`sf` for soft float or whatever) and make 
`/usr/lib` a symlink.  A "mostly 32-bit distro" may have symlink `/usr/lib` -> 
`/usr/lib32`, but still capable to build & run LA64 programs with libraries in 
`/usr/lib64`.  Removing `lib64` will break clang on such distros with 
`-mabi=64`.

Personally, I don't like `lib64`.  But we can't really predict what the distro 
maintainers will do (unless you say something explicitly like "a LP64D capable 
distro SHALL have LP64D libraries in `/usr/lib`" in a spec).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130255

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

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

[Xi Ruoyao via Phabricator via cfe-commits]

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 which of /lib64, /lib has been used. For purity, I'd hope that 
> > we just have /lib, no multilib style /lib64
> I also don't know the actual usage of /lib64 but I just tried and it works 
> fine if I remove /lib64.
I don't like `lib64` too.  But for LoongArch LP64D, the path to ELF interpreter 
is hard coded `/lib64/ld-linux-loongarch-lp64d.so.1` and it seems too late to 
change it.  And LoongArch GCC installs libstdc++ etc. for LP64D into 
$PREFIX/lib64 by default (like x86_64).

As a distro (LFS) maintainer: we are already hacking GCC code to get rid of 
`/usr/lib64`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130255

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