[PATCH] D59744: Fix i386 ABI "__m64" type bug

craig.topper added a comment. -O0 always inline isn't working because the frontend is emitting a store of vector type to memory then a load of x86_mmx to do the type coercion. The caller does the opposite to coerce back from mmx. This -O0 pipeline isn't capable of getting rid of these

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. > $ bin/clang -m32 -O0 /tmp/a.c && ./a.out > -nan > > Before your change, it prints 3.14. I looked through the Intel manual to understand what's happening in detail: When we return from f() with the new ABI, we write to the %mm0 register, and as a side effect:

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. I've reverted in r363790 until a solution can be found. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. In D59744#1549746 , @hans wrote: > In D59744#1549675 , @wxiao3 wrote: > > > Can anyone provide me some small reproducers code for the issue tripped > > over by Chromium / Skia? > > > Sorry, I

[PATCH] D59744: Fix i386 ABI "__m64" type bug

RKSimon added a comment. I've raised https://bugs.llvm.org/show_bug.cgi?id=42319 which suggests the creation of a EMMS insertion pass. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. In D59744#1549675 , @wxiao3 wrote: > Can anyone provide me some small reproducers code for the issue tripped over > by Chromium / Skia? Sorry, I don't have a small repro yet. I'm still working on finding out exactly what's

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Can anyone provide me some small reproducers code for the issue tripped over by Chromium / Skia? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___

[PATCH] D59744: Fix i386 ABI "__m64" type bug

efriedma added a comment. > I'm just laying out the basic requirements for getting this patch back in, > because the current patch is invalid given LLVM's current requirements. Yes, I'm on the same page. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall added a comment. In D59744#1549229 , @efriedma wrote: > If we're going to insert emms instructions automatically, it doesn't really > make sense to do it in the frontend; the backend could figure out the most > efficient placement itself. (See

[PATCH] D59744: Fix i386 ABI "__m64" type bug

efriedma added a comment. If we're going to insert emms instructions automatically, it doesn't really make sense to do it in the frontend; the backend could figure out the most efficient placement itself. (See lib/Target/X86/X86VZeroUpper.cpp, which implements similar logic for AVX.) The

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall added a comment. Thank you. So it sounds like this patch needs to be reverted, and the correct version of it will have to insert these intrinsic calls in four places: - before translating vector arguments to MMX type before calls that pass `__m64` arguments, - after translating MMX

[PATCH] D59744: Fix i386 ABI "__m64" type bug

mtklein added a comment. Ah, thank you for that explanation. That's got to be exactly what we're tripping over in Chromium / Skia. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744

[PATCH] D59744: Fix i386 ABI "__m64" type bug

efriedma added a comment. > Are you saying that using MMX in LLVM requires source-level workarounds in > some way, and so we can't lower portable code to use MMX because that code > will (reasonably) lack those workarounds? Yes. The x86 architecture requires that a program executes an "emms"

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall added a comment. In D59744#1548919 , @efriedma wrote: > > Now, we could theoretically use a different ABI rule for vectors defined > > with Clang-specific extensions, but that seems like it would cause quite a > > few problems of its own. > > I

[PATCH] D59744: Fix i386 ABI "__m64" type bug

efriedma added a comment. > Now, we could theoretically use a different ABI rule for vectors defined with > Clang-specific extensions, but that seems like it would cause quite a few > problems of its own. I think we can't reasonably impose this ABI rule on vectors defined with

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall added a comment. In D59744#1548540 , @mtklein wrote: > Hey folks, I'm the Skia point of contact on this, and "luckily" the person > who wrote all the code that got us into this mess. Let me cross post a > couple questions I've had from the

[PATCH] D59744: Fix i386 ABI "__m64" type bug

mtklein added a comment. Hey folks, I'm the Skia point of contact on this, and "luckily" the person who wrote all the code that got us into this mess. Let me cross post a couple questions I've had from the Chromium bug over here where folks might know the answer... Now that Clang's decided

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. In D59744#1547445 , @wxiao3 wrote: > @hans > > Please make sure all Chromium for 32-bit Linux libraries are following System > V ABI (i.e., m64 is passed on mmx register). I suspect that there are some > hand written assembly code

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. I have created a patch for you: https://reviews.llvm.org/D63473 Is it ok? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list

[PATCH] D59744: Fix i386 ABI "__m64" type bug

thakis added a comment. In D59744#1547445 , @wxiao3 wrote: > @hans > > Please make sure all Chromium for 32-bit Linux libraries are following System > V ABI (i.e., m64 is passed on mmx register). I suspect that there are some > hand written assembly

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. @hans Please make sure all Chromium for 32-bit Linux libraries are following System V ABI (i.e., m64 is passed on mmx register). I suspect that there are some hand written assembly code in your libraries which is not following the ABI. Repository: rL LLVM CHANGES

[PATCH] D59744: Fix i386 ABI "__m64" type bug

hans added a comment. I tried replying on the cfe-commits email, but both Pengfei and Wei's email addresses seem to bounce, so replying here instead: This broke Chromium for 32-bit Linux: https://bugs.chromium.org/p/chromium/issues/detail?id=974542#c5 It's not clear what's happening yet, bet

[PATCH] D59744: Fix i386 ABI "__m64" type bug

This revision was automatically updated to reflect the committed changes. Closed by commit rL363116: [X86] [ABI] Fix i386 ABI __m64 type bug (authored by pengfei, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Thanks for the comments! Updated for landing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 204197. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/x86_32-arguments-linux.c test/CodeGen/x86_32-m64.c Index: test/CodeGen/x86_32-m64.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Minor comments, then LGTM. Comment at: lib/CodeGen/TargetInfo.cpp:1094 +// FreeBSD) don't want to spend any effort dealing with the ramifications +// of ABI

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Thanks for the suggestions! I have updated it. Ok now? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 204057. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/x86_32-arguments-linux.c test/CodeGen/x86_32-m64.c Index: test/CodeGen/x86_32-m64.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1005 bool IsMCUABI; + bool IsLinuxABI; unsigned DefaultNumRegisterParameters; mgorny wrote: > Maybe replace the two booleans with something alike `IsPassInMMXRegABI`? And > while at

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Thanks for the suggestions! I have updated the patch accordingly. Ok for merge now? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 203492. wxiao3 edited the summary of this revision. wxiao3 added a reviewer: mgorny. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/x86_32-arguments-linux.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

mgorny added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1005 bool IsMCUABI; + bool IsLinuxABI; unsigned DefaultNumRegisterParameters; Maybe replace the two booleans with something alike `IsPassInMMXRegABI`? And while at it, include NetBSD

[PATCH] D59744: Fix i386 ABI "__m64" type bug

joerg added a comment. I think MMX code is obscure enough at this point that it doesn't matter much either way. Even less across DSO boundaries. That's why I don't really care either way. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744

[PATCH] D59744: Fix i386 ABI "__m64" type bug

krytarowski added a comment. In D59744#1529218 , @mgorny wrote: > In D59744#1529182 , @krytarowski > wrote: > > > In D59744#1527412 , @wxiao3 wrote: > > > > > Consider

[PATCH] D59744: Fix i386 ABI "__m64" type bug

mgorny added a comment. In D59744#1529182 , @krytarowski wrote: > In D59744#1527412 , @wxiao3 wrote: > > > Consider other Systems (e.g Darwin, PS4 and FreeBSD) don't want to spend > > any effort dealing with the

[PATCH] D59744: Fix i386 ABI "__m64" type bug

krytarowski added subscribers: mgorny, joerg. krytarowski added a comment. In D59744#1527412 , @wxiao3 wrote: > Consider other Systems (e.g Darwin, PS4 and FreeBSD) don't want to spend any > effort dealing with the ramifications of ABI breaks (as

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Consider other Systems (e.g Darwin, PS4 and FreeBSD) don't want to spend any effort dealing with the ramifications of ABI breaks (as discussed in https://reviews.llvm.org/D60748) at present, I only fix the bug for Linux. If other system wants the fix, the only thing

[PATCH] D59744: Fix i386 ABI "__m64" type bug

krytarowski added a comment. sysv abi is not only for UNIX but most non-Windows ones (BSDs, HAIKU, ...). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Hi all, With the latest version, I have made below changes according to all your comments: 1. Only apply the fix to Linux where many libraries are built by GCC. 2. Avoid converting the QualType to llvm::Type and then checking if the llvm::Type is a 64-bit vector, which

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 202579. wxiao3 edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/x86_32-arguments-linux.c test/CodeGen/x86_32-m64.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 marked an inline comment as done. wxiao3 added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:919 /// IsX86_MMXType - Return true if this is an MMX type. bool IsX86_MMXType(llvm::Type *IRType) { - // Return true if the type is an MMX type <2 x i32>, <4 x i16>,

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 199233. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/vector.c test/CodeGen/x86_32-arguments-darwin.c test/CodeGen/x86_32-arguments-linux.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rnk added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:919 /// IsX86_MMXType - Return true if this is an MMX type. bool IsX86_MMXType(llvm::Type *IRType) { - // Return true if the type is an MMX type <2 x i32>, <4 x i16>, or <8 x i8>. wxiao3 wrote:

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 marked 2 inline comments as done. wxiao3 added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:919 /// IsX86_MMXType - Return true if this is an MMX type. bool IsX86_MMXType(llvm::Type *IRType) { - // Return true if the type is an MMX type <2 x i32>, <4 x i16>,

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rnk added subscribers: dexonsmith, rjmccall. rnk added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:919 /// IsX86_MMXType - Return true if this is an MMX type. bool IsX86_MMXType(llvm::Type *IRType) { - // Return true if the type is an MMX type <2 x i32>, <4 x

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 195660. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/vector.c test/CodeGen/x86_32-arguments-darwin.c test/CodeGen/x86_32-arguments-linux.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 195659. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/x86_32-arguments-linux.c test/CodeGen/x86_32-mmx-linux.c Index: test/CodeGen/x86_32-mmx-linux.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

RKSimon added a comment. One last style comment from me but we need somebody better with the different ABIs to finally approve this. Comment at: lib/CodeGen/TargetInfo.cpp:1416 + return ABIArgInfo::getDirect(llvm::Type::getX86_MMXTy(getVMContext())); +} +

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 195291. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/vector.c test/CodeGen/x86_32-arguments-darwin.c test/CodeGen/x86_32-arguments-linux.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

RKSimon added inline comments. Comment at: test/CodeGen/x86_32-m64-darwin.c:1 +// RUN: %clang_cc1 -w -fblocks -triple i386-apple-darwin9 -target-cpu yonah -target-feature +mmx -emit-llvm -O2 -o - %s | FileCheck %s + You should be able to merge all of these

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 updated this revision to Diff 195096. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/vector.c test/CodeGen/x86_32-arguments-darwin.c test/CodeGen/x86_32-arguments-linux.c

[PATCH] D59744: Fix i386 ABI "__m64" type bug

RKSimon added inline comments. Comment at: test/CodeGen/x86_32-mmx-linux.c:2 +// RUN: %clang_cc1 -w -O2 -fblocks -triple i386-pc-linux-gnu -target-cpu pentium4 -emit-llvm -o %t %s +// RUN: FileCheck < %t %s + Test on more triples and add the test file to trunk

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rnk added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:9473-9474 IsWin32FloatStructABI, CodeGenOpts.NumRegisterParameters)); +} else if (Triple.getOS() == llvm::Triple::Linux) { + // System V i386 ABI requires __m64 value passing by MMX registers.

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 added a comment. Dear reviewers, any comments? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59744/new/ https://reviews.llvm.org/D59744 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59744: Fix i386 ABI "__m64" type bug

wxiao3 created this revision. wxiao3 added reviewers: annita.zhang, LuoYuanke, smaslov, craig.topper, hjl.tools. Herald added a project: clang. Herald added a subscriber: cfe-commits. According to System V i386 ABI: the "__m64" type paramater and return va passed by MMX registers. But current