I do not think there is a sensible way to keep upgrade-datalayout2.ll working, with the way the upgrade logic is structured, and we should rethink that test. The change here intends to insert -i128:128- into x86 data layouts that do not have it. The goal of upgrade-datalayout2.ll is to test that data layouts that are not valid x86 data layouts do not get upgraded. However, I see no sensible logic by which we can say that in this particular case, we should not add it.

Rebased.

In D86310#4648634, @nikic wrote:

I'm happy to sign off on the x86-64 part here, but I'm less sure about x86-32. If I understood correctly, the i128 alignment is raised there exclusively to fix the "f128 legalized to i128 libcall" case -- is there any other ABI requirement for i128 alignment on x86-32? Is raising i128 alignment the right way to fix an f128 issue?

In D86310#4648446, @tmgross wrote:

Is this just waiting for a review?

In D124406#4646976, @RKSimon wrote:

@hvdijk reverse-ping - what happended with this patch?

In D86310#4605475, @tmgross wrote:

In D86310#4597359, @hvdijk wrote:

In D86310#4596841, @tmgross wrote:

I think that D158169 seems to have fixed clang as well; after applying both patches, clang gcc and rustc all seem to agree.

Interesting. I cannot see how it would, I may be missing something; I will check when I am able.

D158169 landed today, I confirmed that the current main (with D158169) makes Clang <-> GCC works but LLVM still fails without this patch.

In D86310#4596841, @tmgross wrote:

I think that D158169 seems to have fixed clang as well; after applying both patches, clang gcc and rustc all seem to agree.

In D86310#4596712, @tmgross wrote:

Is clang still doing something wrong? From my testing, it seems like clang and GCC now agree with each other, I am not sure what would still be incorrect

In D86310#4595996, @tmgross wrote:

@nikic posted a patch that fixes the register passing at https://reviews.llvm.org/D158169. I think that patch plus this one should resolve all the problems we have

In D86310#4519549, @tmgross wrote:

Does this happen on the clang side or the LLVM side?

In D86310#4516876, @pengfei wrote:

Just FYI. There are a few reports about the compatibility issues, e.g., #41784.

In D86310#4516184, @tmgross wrote:

Is the compatibility note here only meant to address calls between LLVM and GCC, not generated code? Because of course, struct layout and pass-in-memory function calls are incompatible.

THE CURRENT STATE

Rebased on current LLVM. No longer limited to 64 bit mode. 32 bit mode was taken out in D28990 because it was not believed to be required for compatibility, but as PR50198 shows, it does cause a compatibility issue to leave that as is.

In D86310#4501240, @rnk wrote:

In D86310#4501170, @hvdijk wrote:

For example, it would generally be better if long double were 8-byte-aligned, but the x86 32-bit ABI specifies that it is 4-byte-aligned, and that is set in stone. I would be against any change in LLVM's ABI that changed their alignment, even if it would speed up code.

That may be your view, but other users rely on the -malign-double flag (D19734) to get the new behavior, despite the ABI concerns. Specifically, it mattered for users passing structs from CPU to GPU, because the GPU doesn't tolerate misaligned doubles well. With that in mind, I wouldn't describe this ABI rule as being "set in stone", but I understand your perspective.

@JohnReagan That is a valid concern, and I hope it reassures you that if things were working before, I would never be on board with this change. For example, it would generally be better if long double were 8-byte-aligned, but the x86 32-bit ABI specifies that it is 4-byte-aligned, and that is set in stone. I would be against any change in LLVM's ABI that changed their alignment, even if it would speed up code. I still occasionally run 20-year-old binaries, myself, that are dynamically linked to shared object files built with current compilers. Compatibility matters, I would not be on board with a change that breaks things like that. But that is not what is happening here. For i128, what clang implemented matched GCC, what LLVM implemented deviated from GCC/clang, but LLVM assumed that its implementation actually did match GCC/clang and code crashed as a result. This change would make it so that LLVM starts to also match GCC/clang, to change things from something that doesn't work to something that does work, and because things crash in current LLVM, I do not believe there can be much code out there that relies on the current behaviour. As you say, you aren't using i128/f128 yourself either. I hope that when I can update the patch, you can check that this does not cause problems for you.

In D86310#4498575, @efriedma wrote:

https://reviews.llvm.org/D86310#2231136 has an example where IR generated by clang breaks.

In D86310#4498551, @rnk wrote:

Given that most non-clang frontends want the bug fix (ABI break), who exactly is asking for this level of IR ABI stability?

In D86310#4496582, @nikic wrote:

The main problem with that is that we can't have multiple data layouts for one module, so linking old and new bitcode together would fail.

A thought occurs: in older versions of LLVM, the data layout mechanism worked differently and permitted targets to declare that they supported multiple different data layout strings, by overriding isCompatibleDataLayout. This mechanism was removed in D67631. If we reinstate that, we can have the X86 target declare that it "supports" data layout strings with and without the -i128:128, where by "supports", I mean the code continues to not generally work in the same way it does not generally work now, but the specific limited cases that do work continue to work exactly the same ABI-incompatible way. This would have the same result of bug-for-bug compatibility with existing modules, but in what I suspect would be a significantly simpler way than by going through the module and adding explicit alignments everywhere. While I would still prefer to give up on that compatibility, if it is a hard requirement, and if this would be an alternative way of achieving it, I might possibly be able to update this patch to do just that. Would this be acceptable?

In D86310#4485837, @tmgross wrote:

What is the current status of this patch? Are the reviewers here OK with this fix in general but just need to see changes to autoupgrade?

https://github.com/llvm/llvm-project/issues/62194

In rG5ad2aae24474bb5366e84bfe27a5052185d856ce#1195619, @MaskRay wrote:

In my https://sourceware.org/pipermail/binutils/2022-November/124312.html example, fuse should simply remove FUSE_SYMVER(".symver fuse_new_compat2,fuse_new@");, which will remove the problematic unversioned fuse_new symbol.

In rG5ad2aae24474bb5366e84bfe27a5052185d856ce#1142817, @hvdijk wrote:

In rG5ad2aae24474bb5366e84bfe27a5052185d856ce#1142816, @MaskRay wrote:

FYI: I CCed you on https://sourceware.org/pipermail/binutils/2022-November/124312.html

Appreciated. Will wait and see what they say.

In D140315#4077611, @scchan wrote:

In D140315#4076614, @mgorny wrote:

I'm sorry for reporting it this late (clang's been broken for over two weeks, so we couldn't periodically test it) but this change is causing test regressions on Gentoo/amd64:

Thanks for reporting the issue. I'm working on a patch to fix the test.

In D110900#4242228, @tstellar wrote:

This is exactly what we are doing in Fedora right now: https://src.fedoraproject.org/rpms/clang/blob/rawhide/f/0001-Driver-Add-a-gcc-equivalent-triple-to-the-list-of-tr.patch.

In D110900#4242187, @MaskRay wrote:

Can you elaborate? I agree that decreasing the number of filesystem stats is preferable...

In D110900#3044136, I suggested another approach that should reduce the number of file system accesses, which you implemented in D110900#3044545 (D111207). Apologies for never responding to that; at first glance the way you implemented that looks like it increases the number of file system accesses, and I don't think we should do that. Given that the only implemented alternative is this one, I'm not going to argue against this.

Forgot to update the call to foo() to call bar() instead.

Apparently the test was already passing without my change, so I updated the test to make it a function that did not previously get the nounwind attribute. This also revealed that the test was more fragile than I realised: it depends on the precise order in which functions are emitted. We always match "Function Attrs:" to the first emitted function, and then CHECK-NEXT can fail. We'd ideally first CHECK for void @_Z3foov() (now void @_Z3barv()) and then CHECK-PREV for the attributes, but FileCheck does not support that. What we can do instead though is find the matching attributes #n = { ... } line and verify that everything is in there; updated accordingly.

That's a good question. I haven't looked into this issue deep enough, but I think using -fexceptions requires using delayed diagnostics to avoid false diagnostics during host code analysis.

Is the rationale I gave in the description correct, or would it be better for SYCL device code to unconditionally build without -fexceptions and get the nounwind attribute added that way?

In rG5ad2aae24474bb5366e84bfe27a5052185d856ce#1142816, @MaskRay wrote:

In rG5ad2aae24474bb5366e84bfe27a5052185d856ce#1142807, @hvdijk wrote:

Your last comment reads like things only break if FUSE 2 is linked with ld.bfd, and ntfs3g with ld.lld, that ld.bfd is able to produce something that ld.lld is unable to process as intended. For the avoidance of doubt, on my system, FUSE 2 was built with clang+lld, so any scenario that lld cannot produce is not something that could have come up.

No, things only break if libfuse 2 is linked with ld.lld (which cannot produce VER_NDX_GLOBAL VERSYM_HIDDEN) and has fuse_new@@FUSE_2.6 before fuse_new (VER_NDX_GLOBAL) and a program links against libfuse.so.

Your last comment reads like things only break if FUSE 2 is linked with ld.bfd, and ntfs3g with ld.lld, that ld.bfd is able to produce something that ld.lld is unable to process as intended. For the avoidance of doubt, on my system, FUSE 2 was built with clang+lld, so any scenario that lld cannot produce is not something that could have come up.

In rG5ad2aae24474bb5366e84bfe27a5052185d856ce#1142464, @MaskRay wrote:

Note that in the version on my system, fuse_new appears after fuse_new@@FUSE_2.6, whereas in the version on your system, it appears before. I have not checked in detail what is happening in this commit. Is it possible that for whatever reason it is causing whichever version is seen last to take priority? If so, it is unfortunate that you will not be able to reproduce the problem on your system with the Debian-provided version of FUSE. I should be able to come up with a standalone test that does not rely on FUSE, but it will take longer.

It's difficult to tell without a standalone reproducer...

The symbols in the DSO are:

% readelf -W --dyn-syms /lib/x86_64-linux-gnu/libfuse.so | grep fuse_new

200: 00000000000148b0    72 FUNC    GLOBAL DEFAULT   13 fuse_new@FUSE_2.5
201: 0000000000014870    16 FUNC    GLOBAL DEFAULT   13 fuse_new
202: 0000000000014750     8 FUNC    GLOBAL DEFAULT   13 fuse_new@@FUSE_2.6
203: 0000000000014860    11 FUNC    GLOBAL DEFAULT   13 fuse_new@FUSE_2.2
216: 0000000000014880    35 FUNC    GLOBAL DEFAULT   13 fuse_new_compat1@@FUSE_2.2
218: 0000000000014870    16 FUNC    GLOBAL DEFAULT   13 fuse_new_compat2@@FUSE_2.2
228: 0000000000014860    11 FUNC    GLOBAL DEFAULT   13 fuse_new_compat22@@FUSE_2.5
230: 00000000000148b0    72 FUNC    GLOBAL DEFAULT   13 fuse_new_compat25@@FUSE_2.6

This was committed as NFC, but isn't NFC and did end up breaking code.

Thanks, I am seeing improvements with this new version. I'll try to go over the changes in more detail later, some initial superficial comments now.

In D127119#3564522, @ABataev wrote:

In D127119#3564461, @hvdijk wrote:

Comparing the changes to the tests in this diff to those in D127073, I am seeing a number of tests where we have more shufflevectors, and none where we have fewer. Are there improvements that are not as obvious to see?

Yes, there are. These extra shuffles caused by changes in performExtractsShuffleAction() and in IsIdenticalOrLessDefined lambda, these changes (they treat UndefMaskElem as possible poison) increase number of shuffles. Without them, there are less shuffles, these extra changes are required for correct handling of UndefMaskElem as posion.

Comparing the changes to the tests in this diff to those in D127073, I am seeing a number of tests where we have more shufflevectors, and none where we have fewer. Are there improvements that are not as obvious to see?

The updates to the tests suggest that this doesn't just fix the broken cases, it generates worse code where the previously generated code was already correct: I'm seeing insertelements into a vector immediately followed by a shufflevector that discards the inserted element, where previously those elements were not set. Is there some way that we can avoid that?

In D126895#3554064, @vporpo wrote:

I think that the issue boils down to whether: %zext = zext i8 poison to i32 is an i32 poison.

The value coming from an unreachable block from entry is uninitialized

In D122663#3471741, @erichkeane wrote:

Ping me EOW if @rsmith doesn't respond in the meantime.

Add a comment explaining why we do not use address size overrides here.

Clean up test slightly to load consecutive i32 values like the i64 version did

In D122663#3471866, @erichkeane wrote:

This actually wouldn't be necessary in this case: cxxfilt is already 'right', this is just for humans to be able to see "we changed this from mangling as <obviously wrong> to <looks right>".

In D122663#3471763, @erichkeane wrote:

I DID see that and am REASONABLY sure you do, but sometimes folks will ask for test coverage because they suspect the resulting behavior will demonstrate some sort of problem with the current patch. I DID run this through the demangler and found that the PRE15 case looks odd?

void test10<X>(X::Y::a, X::Y::b, float*, X::Y)
vs
void test10<X>(X::Y::a, X::Y::b, float*, float*)

Or is that exactly the bug being caught here? That is, is the float* substitution not being properly registered/emitted and being confused with X::Y?

In D122663#3471741, @erichkeane wrote:

Ping me EOW if @rsmith doesn't respond in the meantime. It is also not clear to me whether you were able to capture/fix the issue he had with the clang-abi-compat.cpp test.

In D122663#3457330, @erichkeane wrote:

LGTM! I would like @rjmccall to take a pass if he ends up having time in the next day or two (perhaps tack on an extra day or two because of Easter), else I'll be willing to approve later in the week.

Fixed release notes to use correct RST syntax.

Extend test, add assert.

Add to release notes.

In D122663#3457131, @erichkeane wrote:

Our most recent direction is to document any non-NFC patches in the release notes if at all sensible, so I think this meets those requirements.

In D122663#3456507, @erichkeane wrote:

I believe the answer here is 'yes'. We also likely need release notes.

Allow the old mangling with suitable -fclang-abi-compat= value

ping. Apologies, I don't know who to add as a reviewer, there is no one specifically listed as code owner and it does not seem to be handled by anyone in particular. @urnathan, @erichkeane, you two appear to be the most recent people who pushed commits specifically for mangling issues, would either of you be able to review this?

I've pushed D122540, would you like me to push this as well or would you rather do that yourself?

I mentioned that I'd prefer to get another person to review this as well but in the interest of just getting the bug fixed, I'll merge this tomorrow if there is no more feedback.