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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG37d1d02200b9: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match… (authored by pengfei). Repository: rG LLVM Github

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

pengfei updated this revision to Diff 400231. pengfei added a comment. Fix AutoUpgrade bug catched by buildbot. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942 Files: clang/lib/Basic/Targets/X86.h

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

pengfei added a comment. It caused by bug in AutoUpgrade.cpp, fixed on D117270 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942

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

pengfei added a comment. I see, I will investigate the reason and do a follow up. Thanks @efriedma and @rnk ! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942

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

rnk added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/

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

efriedma added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942

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

pengfei added a comment. Should be fixed by rG9b43237128da . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942

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

pengfei added a comment. Thanks @fhahn ! I saw the failure. I think it just needs to update the datalayout in the lit tests. I am checking on this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942

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

fhahn added a comment. 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';

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1bb0caf56168: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match… (authored by pengfei). Repository: rG LLVM Github

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

pengfei added a comment. Thanks Craig! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

craig.topper accepted this revision. craig.topper added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942 ___ cfe-commits mailing list

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

pengfei added inline comments. Comment at: llvm/lib/IR/AutoUpgrade.cpp:4583-4589 + if (T.isArch64Bit() || !T.isWindowsMSVCEnvironment()) +return Res; - return (Groups[1] + AddrSpaces + Groups[3]).str(); + StringRef Ref = Res; + auto I = Ref.find("-f80:32-"); + if (I

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

pengfei updated this revision to Diff 399235. pengfei marked an inline comment as done. pengfei added a comment. Add comments in code. @rnk, thanks for your patient review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. 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. Comment at:

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

pengfei updated this revision to Diff 398335. pengfei added a comment. Missed one unittest. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942 Files: clang/lib/Basic/Targets/X86.h

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

pengfei updated this revision to Diff 398334. pengfei added a comment. Herald added a subscriber: dexonsmith. > 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

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

rnk added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/

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

rnk added a comment. In D115942#3227047 , @pengfei wrote: > In D115942#3226146 , @rnk wrote: > >> Yeah, let's try to reach some resolution on that. > > The things are different. We don't support f80 type on

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

pengfei updated this revision to Diff 398123. pengfei added a comment. Structure the layout spelling. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942 Files: clang/lib/Basic/Targets/X86.h

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

pengfei added a comment. In D115942#3226146 , @rnk wrote: > 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

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

pengfei added a comment. In D115942#3226101 , @craig.topper wrote: > 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

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

rnk requested changes to this revision. rnk added a comment. This revision now requires changes to proceed. 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

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

craig.topper added a subscriber: efriedma. craig.topper added a comment. Does this have the same autoupgrade issues as @efriedma raised in https://reviews.llvm.org/D86310 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/

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

rnk accepted this revision. rnk added subscribers: jyknight, thakis. rnk added a comment. This revision is now accepted and ready to land. Thanks, looks good. I have a minor code refactoring suggestion, but feel free to resolve it as you see fit and land it. Comment at:

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

pengfei added a comment. This is split from D115441 . Add a targeted LLVM test. Thanks @rnk for the advise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D115942

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

pengfei created this revision. pengfei added reviewers: rnk, andrew.w.kaylor, erichkeane, craig.topper. Herald added a subscriber: hiraditya. pengfei requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. MSVC currently doesn't