MSVC currently doesn't support 80 bits long double. ICC supports it when
the option /Qlong-double is specified. Changing the alignment of f80
to 16 bytes so that we can be compatible with ICC's option.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks, looks good. I have a minor code refactoring suggestion, but feel free to resolve it as you see fit and land it.
clang/lib/Basic/Targets/X86.h | ||
---|---|---|
534–535 | I think it would be simpler to make the MSVC mode data layout change here. It is also my understanding that, prior to rGb214cbc7852fa1719c5d0b, only long-lived string constants could be passed to resetDataLayout. Since 2016, that is unneeded. resetDataLayout now copies its string argument. (@jyknight @thakis, who made the changes here). I suggest structuring the code like this: bool IsWinCOFF = getTriple()... bool IsMSVC = getTriple()... StringRef Mangling = IsWinCOFF ? "m:x" : "m:e"; StringRef F80 = IsMSVC ? "f80:128" : "f80:32"; std::string Layout = "e-" + Mangling + "-p:32..." + F80 + "-n8:..."; resetDataLayout(Layout, IsWinCOFF ? "_" : ""); |
Does this have the same autoupgrade issues as @efriedma raised in https://reviews.llvm.org/D86310
Yeah, let's try to reach some resolution on that.
In the mean time, I discovered the alignstack parameter attribute:
https://llvm.org/docs/LangRef.html#parameter-attributes
Could that be used to solve this problem in the frontend instead?
No. The differences are that i128 can be generated by front end while f80 cannot without D115441 in the given datalayout (i.e., MSVC).
I also did some experiments, e.g., generating a bc file (without f80 type) with existing compiler and compile it with the one with this patch. There's no error during compilation. So I think we don't bother to do autoupgrade at all.
The things are different. We don't support f80 type on Windows 32 bits previously. It means we don't have the burden to upgrade, since there's no global/load/store/GEP/etc for f80.
In the mean time, I discovered the alignstack parameter attribute:
https://llvm.org/docs/LangRef.html#parameter-attributesCould that be used to solve this problem in the frontend instead?
It might be feasible, but I don't think it's a good idea. It looks to me more like a language specific alignment hint instead of medium that carrying target specific information.
However, do we need to make changes to llvm::UpgradeDataLayoutString? Otherwise, I believe old bitcode for MSVC targets will no longer be able to be loaded for LTO.
However, do we need to make changes to llvm::UpgradeDataLayoutString? Otherwise, I believe old bitcode for MSVC targets will no longer be able to be loaded for LTO.
Makes sense. Done, thanks!
I'm happy with this, but I'd like to get an ack from @craig.topper or @efriedma about the backwards compatibility concern. I think it's addresed.
llvm/lib/IR/AutoUpgrade.cpp | ||
---|---|---|
4583–4589 ↗ | (On Diff #398335) | I think the early return here is not helping readability. Please add a comment here about why this upgrade is being done, something like: // For 32-bit MSVC targets, raise the alignment of f80 values to 16 bytes. Raising the alignment is safe because Clang did not produce f80 values in the MSVC environment before this upgrade was added. if (T.isWindowsMSVCEnvironment() && T.isArch32Bit()) { .... |
Add comments in code. @rnk, thanks for your patient review!
llvm/lib/IR/AutoUpgrade.cpp | ||
---|---|---|
4583–4589 ↗ | (On Diff #398335) | That's better. Thanks for the suggestion. |
It looks like this may be causing the following bot to fail https://lab.llvm.org/buildbot/#/builders/16/builds/22138 with
******************** TEST 'lld :: COFF/lto-lazy-reference.ll' FAILED ******************** Script: -- : 'RUN: at line 2'; /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llc -mtriple=i686-pc-windows-msvc -filetype=obj -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-quadruple.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/lld/test/COFF/Inputs/lto-lazy-reference-quadruple.ll : 'RUN: at line 3'; /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-as -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-dummy.bc /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/lld/test/COFF/Inputs/lto-lazy-reference-dummy.ll : 'RUN: at line 4'; rm -f /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib : 'RUN: at line 5'; llvm-ar cru /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-quadruple.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-dummy.bc : 'RUN: at line 6'; /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-as -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/lld/test/COFF/lto-lazy-reference.ll : 'RUN: at line 7'; /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/lld-link /out:/b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.exe /entry:main /subsystem:console /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib -- Exit Code: 134 Command Output (stderr): -- + : 'RUN: at line 2' + /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llc -mtriple=i686-pc-windows-msvc -filetype=obj -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-quadruple.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/lld/test/COFF/Inputs/lto-lazy-reference-quadruple.ll + : 'RUN: at line 3' + /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-as -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-dummy.bc /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/lld/test/COFF/Inputs/lto-lazy-reference-dummy.ll + : 'RUN: at line 4' + rm -f /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib + : 'RUN: at line 5' + llvm-ar cru /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-quadruple.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference-dummy.bc + : 'RUN: at line 6' + /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/llvm-as -o /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/lld/test/COFF/lto-lazy-reference.ll + : 'RUN: at line 7' + /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/lld-link /out:/b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.exe /entry:main /subsystem:console /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib lld-link: /b/1/llvm-clang-x86_64-expensive-checks-debian/llvm-project/llvm/lib/CodeGen/MachineFunction.cpp:207: void llvm::MachineFunction::init(): Assertion `Target.isCompatibleDataLayout(getDataLayout()) && "Can't create a MachineFunction using a Module with a " "Target-incompatible DataLayout attached\n"' failed. /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.script: line 6: 2469401 Aborted /b/1/llvm-clang-x86_64-expensive-checks-debian/build/bin/lld-link /out:/b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.exe /entry:main /subsystem:console /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.obj /b/1/llvm-clang-x86_64-expensive-checks-debian/build/tools/lld/test/COFF/Output/lto-lazy-reference.ll.tmp.lib --
Thanks @fhahn ! I saw the failure. I think it just needs to update the datalayout in the lit tests. I am checking on this.
Not sure I agree with the "fix" to the lld tests. If the linker is crashing, clearly there's a bug, not just an incorrect test.
Right, the auto-upgrade should upgrade and prevent the need to change the bitcode data layout. LTO is the main use case for auto-upgrading.
I think it would be simpler to make the MSVC mode data layout change here.
It is also my understanding that, prior to rGb214cbc7852fa1719c5d0b, only long-lived string constants could be passed to resetDataLayout. Since 2016, that is unneeded. resetDataLayout now copies its string argument. (@jyknight @thakis, who made the changes here).
I suggest structuring the code like this: