User Details
- User Since
- Aug 13 2019, 4:58 AM (216 w, 1 d)
Thu, Sep 21
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.
Wed, Sep 20
Tue, Sep 19
Mon, Sep 18
Mon, Sep 11
Aug 17 2023
Jul 20 2023
Jul 19 2023
Jul 16 2023
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.
Jul 14 2023
@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.
Jul 13 2023
Jul 12 2023
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?
Jul 10 2023
Apr 17 2023
Apr 12 2023
Apr 4 2023
Apr 3 2023
Apr 2 2023
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.
Mar 29 2023
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.
Mar 28 2023
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?
Mar 9 2023
Nov 10 2022
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.
Nov 9 2022
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.
Jun 28 2022
Jun 12 2022
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.
Jun 7 2022
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?
Jun 6 2022
Jun 5 2022
Jun 4 2022
Jun 3 2022
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?
Jun 2 2022
The value coming from an unreachable block from entry is uninitialized
May 3 2022
May 2 2022
Apr 26 2022
Add a comment explaining why we do not use address size overrides here.
Apr 25 2022
Clean up test slightly to load consecutive i32 values like the i64 version did
Apr 24 2022
Apr 20 2022
Fixed release notes to use correct RST syntax.
Apr 19 2022
Apr 18 2022
Extend test, add assert.
Add to 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?
Apr 12 2022
I've pushed D122540, would you like me to push this as well or would you rather do that yourself?
Apr 11 2022
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.