Page MenuHomePhabricator

[X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC
ClosedPublic

Authored by pengfei on Dec 17 2021, 7:35 AM.

Details

Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
750 msx64 debian > SanitizerCommon-tsan-x86_64-Linux.Linux::decorate_proc_maps.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -m64 -funwind-tables -ldl -g /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/decorate_proc_maps.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/decorate_proc_maps.cpp.tmp

Event Timeline

pengfei created this revision.Dec 17 2021, 7:35 AM
pengfei requested review of this revision.Dec 17 2021, 7:35 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 17 2021, 7:35 AM

This is split from D115441. Add a targeted LLVM test. Thanks @rnk for the advise.

rnk accepted this revision.Jan 6 2022, 1:46 PM
rnk added subscribers: jyknight, thakis.

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 ? "_" : "");
This revision is now accepted and ready to land.Jan 6 2022, 1:46 PM

Does this have the same autoupgrade issues as @efriedma raised in https://reviews.llvm.org/D86310

rnk requested changes to this revision.Jan 6 2022, 2:16 PM

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?

This revision now requires changes to proceed.Jan 6 2022, 2:16 PM

Does this have the same autoupgrade issues as @efriedma raised in https://reviews.llvm.org/D86310

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.

Yeah, let's try to reach some resolution on that.

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

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

pengfei updated this revision to Diff 398123.Jan 7 2022, 6:13 AM

Structure the layout spelling.

pengfei marked an inline comment as done.Jan 7 2022, 6:13 AM
rnk added a comment.Jan 7 2022, 3:41 PM

Yeah, let's try to reach some resolution on that.

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.

Good point, I buy that argument.

rnk added a comment.Jan 7 2022, 3:45 PM

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.

pengfei updated this revision to Diff 398334.Jan 8 2022, 6:24 AM

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!

pengfei updated this revision to Diff 398335.Jan 8 2022, 6:40 AM

Missed one unittest.

rnk accepted this revision.Jan 11 2022, 12:17 PM

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
4588–4594

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()) {
 ....
This revision is now accepted and ready to land.Jan 11 2022, 12:17 PM
pengfei updated this revision to Diff 399235.Jan 12 2022, 12:22 AM
pengfei marked an inline comment as done.

Add comments in code. @rnk, thanks for your patient review!

llvm/lib/IR/AutoUpgrade.cpp
4588–4594

That's better. Thanks for the suggestion.

Thanks Craig!

This revision was landed with ongoing or failed builds.Jan 12 2022, 1:50 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Jan 12 2022, 2:47 AM

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.

rnk added a comment.Jan 12 2022, 11:19 AM

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 see, I will investigate the reason and do a follow up. Thanks @efriedma and @rnk !

It caused by bug in AutoUpgrade.cpp, fixed on D117270.

pengfei reopened this revision.Jan 14 2022, 6:56 PM
This revision is now accepted and ready to land.Jan 14 2022, 6:56 PM
pengfei updated this revision to Diff 400231.Jan 14 2022, 7:37 PM

Fix AutoUpgrade bug catched by buildbot.

This revision was landed with ongoing or failed builds.Jan 22 2022, 5:59 PM
This revision was automatically updated to reflect the committed changes.