[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3e19ba36fca9: [X86][MS] Add 80bit long double support for Windows (authored by pengfei). Changed prior to commit: https://reviews.llvm.org/D115441

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk accepted this revision. rnk added a comment. lgtm I believe you can go ahead and land this, it doesn't depend on the data layout changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115441/new/ https://reviews.llvm.org/D115441

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei updated this revision to Diff 395129. pengfei added a comment. Split the LLVM datalayout to a different patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115441/new/ https://reviews.llvm.org/D115441 Files: clang/lib/Basic/TargetInfo.c

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei updated this revision to Diff 395128. pengfei added a comment. Split the LLVM datalayout to a different patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115441/new/ https://reviews.llvm.org/D115441 Files: clang/lib/Basic/TargetInfo.c

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. I see, thanks for the info. Can you please add a targeted LLVM test for long double arguments? From what I can tell, the auto-generated update_llc_test_checks.py style tests are not a good fit for testing parameter passing because they pattern-match away the stack offsets

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. Hi @rnk , mine is Phoebe#3036. I haven't really used it before. No sure if I invited you correctly, so I try to explain here. > My comment here refers to the alignment of argument values, not user-declared > variables. The frontend controls the alignment of user-declare

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. Let me know if it would be more helpful to set up a call, that might help us reach agreement sooner. I've used discord for this previously if that works for you, my username there is the same (`@rnk#8591`). In D115441#3194066 , @pen

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. > GCC doesn't align fp80 long double to 16 bytes on i686, so I see no reason > for LLVM to do it. Is there some other compiler that you need ABI > compatibility with? Yes. ICC aligns long double to 16 bytes on 32bit Windows. (I mentioned it in the summary :). In contra

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. In D115441#3191482 , @pengfei wrote: > We have to change LLVM data layout because it's required by the calling > conversion. Is that necessary? It would be simpler to leave the fp80 value 4 byte aligned, which I believe is consiste

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. In D115441#3189551 , @JohnReagan wrote: > Does any of this impact the -f128 support? We use f128 x-float on OpenVMS. > We've historically only aligned on 8-byte boundaries for legacy reasons (I'm > not opposed to having my ow

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. > I have a thought. Why do you need to change the LLVM data layout at all? > Clang's record layout is distinct from LLVM's data layout. This is similar to > how -malign-double works, which does not affect LLVM's data layout, it is > entirely a frontend change. We have

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

JohnReagan added a comment. Does any of this impact the -f128 support? We use f128 x-float on OpenVMS. We've historically only aligned on 8-byte boundaries for legacy reasons (I'm not opposed to having my own mods to control the record layout and/or data layout) Repository: rG LLVM Github

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. In D115441#3188526 , @mstorsjo wrote: > In D115441#3188172 , @pengfei wrote: > >>> In GCC on Windows (and clang in mingw mode), long double is always 80 bit >>> on x86. (On i386, sizeof(long

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

mstorsjo added a comment. In D115441#3188172 , @pengfei wrote: >> In GCC on Windows (and clang in mingw mode), long double is always 80 bit on >> x86. (On i386, sizeof(long double) == 12, while on x86_64 it's 16.) > > How about the alignment? I can see o

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. Thanks @mstorsjo for the inputs. > However outside of the core OS, any function in the CRT, that uses long > doubles, is going to be wrong Good point! I didn't think much on the CRT library. But I think this is not a blocking issue, given 1. The option is off by defau

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

mstorsjo added a comment. In D115441#3187009 , @rnk wrote: > I seem to recall assuming that Windows `long double` was 64-bits in many, > many places. Unfortunately, I have no idea where that could've happened. Nothing comes to mind for me about that - i

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. > I seem to recall assuming that Windows long double was 64-bits in many, many > places. Unfortunately, I have no idea where that could've happened. Is it because MSDN explicitly declares it? https://docs.microsoft.com/en-us/previous-versions/9cx8xs15(v=vs.140)?redirect

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a subscriber: mstorsjo. rnk added a comment. I seem to recall assuming that Windows `long double` was 64-bits in many, many places. Unfortunately, I have no idea where that could've happened. Something that comes to mind immediately is the MSVC name mangler. I don't think that's a blo

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

pengfei added a comment. In D115441#3182938 , @craig.topper wrote: > Doesn’t icc also emit code into main to change the FPCW precision control > field? Is making long double 80 bits useful if you don’t increase the > precision in hardware? Yes, but it

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

craig.topper added a comment. Doesn’t icc also emit code into main to change the FPCW precision control field? Is making long double 80 bits useful if you don’t increase the precision in hardware? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1154

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This looks good to me, and mirrors something I implemented in our downstream a few years ago. Please don't submit for another day or two to give others a chance to review. Repositor

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

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 s