graphite2: Fix universal build

[tuffnatty]

Description

Fix universal build for graphics/graphite2 which used to wrongly specify SSE flags if the build machine is Intel. There is a pull request in upstream: silnrsi/graphite#84

Type(s)

• bugfix
• enhancement
• security fix

Tested on

macOS 11.7.4
Xcode 13 / Command Line Tools 13.2.0.0.1.1638488800

Verification

Have you

• followed our Commit Message Guidelines?
• squashed and minimized your commits?
• checked that there aren't other open pull requests for the same change?
• referenced existing tickets on Trac with full URL?
• tried a full install with sudo port -vst install?
• tested basic functionality of all binary files?
• checked that the Portfile's most important variants haven't been broken?

[macportsbot]

Notifying maintainers:
@ryandesign for port graphite2.

[ryandesign]

Thanks. Does this fix https://trac.macports.org/ticket/64532? If so, the commit message should say so.

The commit message should begin with portname:, not category/portname:.

Let's see what upstream says about your PR before we accept this.

[tuffnatty]

Thanks. Does this fix https://trac.macports.org/ticket/64532? If so, the commit message should say so.

I hope that it fixes the universal Intel/PowerPC build as well, but I could test only the Intel/ARM64 build. I also understood at first the ticket is about cross-building with Rosetta, not specifically about universal. Added "Probably fixes ..."

The commit message should begin with portname:, not category/portname:.

Thanks! Updated.

Let's see what upstream says about your PR before we accept this.

The upstream seems quite dormant. graphite2 is a dependency of harfbuzz, and harfbuzz a dependency of a lot of great packages we can't build as universal at the moment.
I suppose that the resulting fat binary has some performance degradation on Intel, but I also think this code is never used, as Graphite fonts are very rare. However, let's see.

[barracuda156]

Thanks. Does this fix https://trac.macports.org/ticket/64532? If so, the commit message should say so.

I hope that it fixes the universal Intel/PowerPC build as well, but I could test only the Intel/ARM64 build. I also understood at first the ticket is about cross-building with Rosetta, not specifically about universal. Added "Probably fixes ..."

You could have tagged me, that was my ticket. I will test this in Rosetta, as well as ppc+ppc64.

P. S. I am not really sure about MATCHES syntax, but possibly ppc64 should be here too? ${CMAKE_OSX_ARCHITECTURES} MATCHES "arm|ppc"

[barracuda156]

@ryandesign Yes, it does, but it seems that the patch here is applied to a wrong spot. Obviously, it should be in Darwin chunk and not Linux.

svacchanda$ port -v installed graphite2
The following ports are currently installed:
  graphite2 @1.3.13_1 requested_variants='' platform='darwin 10' archs='ppc' date='2022-08-01T02:23:34+0800'
  graphite2 @1.3.14_0 (active) requested_variants='' platform='darwin 10' archs='ppc' date='2023-04-01T07:39:07+0800'

[tuffnatty]

@barracuda156 Updated the broken patch. It works for me with arm64, so I don't think there's a need to add ppc64.

[barracuda156]

@barracuda156 Updated the broken patch. It works for me with arm64, so I don't think there's a need to add ppc64.

@tuffnatty Cannot verify atm, but I am fine with that: if anything, adding ppc64 will be trivial.

[tuffnatty]

Updated the patch with proper CMake quoting.

[tuffnatty]

Updated the patch to also use muniversal PortGroup avoiding any performance degradation.

[barracuda156]

@tuffnatty Ah, this one is still pending. If not in a hurry to merge, I can test this for ppc+ppc64 in a day or two (machine with 10.5 is in office).

[tuffnatty]

@barracuda156 Thanks!

[barracuda156]

@tuffnatty Will be done no later than tomorrow morning (like, in a few hours). Sorry for a bit of a delay.

UPD. Yes, it works fine on 10.5.8 for ppc + ppc64.

[tuffnatty]

@barracuda156 Thanks!

Rebased the patch against master.

[reneeotten]

@ryandesign upstream hasn't commented on the PR mentioned here where the same fix was suggested. Are you comfortable with merging this? If so, please do... if not let's close the PR.

[pmetzger]

@reneeotten I have a strong suspicion that @ryandesign does not get notifications about being mentioned in PR threads. I'm unsure why.

[kencu]

There are several ports that share this same issue -- conflicting SIMD flags for different arch builds. graphene is another one that comes up https://trac.macports.org/ticket/66888.

Just turning all SIMD flags off in universal builds can work, but it is a somewhat costly approach to take as a whole, long-term, for MacPorts, performance-wise. It does get it fixed quickly, though.

Of interest, when building universal, clang will use neon SIMD flags on arm64 but not on x86_86, and use SSE/etc flags on x86_64, but not on arm64, etc -- in other words -- it will do the right thing if all the SIMD flags are used.

It is designed to do this, specifically for this problem.

gcc used to be able to do that too, when Apple was involved in it ... but that was last as of gcc-4.2, so ancient times now.

It's merging the pkgconfig flags that are the hiccup. So one thing to do rather than turn all the SIMD flags off, is turn them all ON.

Another approach is to have different pkgconfig flags that will be used on different architectures. That way they don't have to be merged. This is actually possible, but would require some tooling in macports to support. This issue has come up before, in other projects, and I put some thoughts about the the approach here last year when I ran across this many times building universal:

https://trac.macports.org/ticket/66894

I think ... turning the SIMD flags all ON and letting clang sort them out is probably the most expedient thing to do. Until gcc is updated, they might have to be turned all off there, but it would be unfortunate to disable all the acceleration for all newer systems all the time to support an ancient architecture virtually nobody builds any longer.

Sorting out arch-specific pkgconfig flags as per https://trac.macports.org/ticket/66894 has a tantalizing elegance about it.... but not sure anyone is going to actually take the bull by the horns and do that project. There are only a few folks around MacPorts with the skill set for a project like that -- Marcus, Josh, Ryan, etc -- and they are all pretty busy right now.

[barracuda156]

@kencu As long as turning all SIMD flags on is clang-only, and for gcc builds they are either set correctly per-arch or at least turned off for all universal builds, all should be good.

[pmetzger]

So can we finish this?

[kencu]

If this is rushed, the temptation would be to just disable all SIMD when building universal, and that (IMHO) would be the wrong thing to do.

Imagine sorting out build or functional issues when the library you're linking against is built differently depending on whether it was installed universal or not. Nightmare.

It needs to be sorted out properly, and what is done here will be done elsewhere.

[tuffnatty]

@kencu wrote:

@tuffnatty … in your own tree, you decided disabling all SIMD was fine for you, and so you’re good, you have a universal graphite2, you’ve moved on.

This is not a very accurate description of what my patch does. My patch does not break or slow down any build that was working well before the patch. It only disables passing wrong flags in a cross-platform build situation. Pure PowerPC and ARM builds did never use any SIMD flags. Only when building on Intel for a non-native target, Intel SIMD flags were being passed erroneously. That is, the SIMD flags were ever used by upstream only on Linux+Intel, Darwin+Intel graphite2 builds, ignoring other OSes and CPUs.

By using muniversal, the Intel part of the fat binary is built with SIMD flags, and the ARM/PowerPC part is built without, but SIMD was never supported by upstream for PowerPC anyway, and for ARM64, SIMD support is on by default. Probably if someone could recommend proper PowerPC flags to me (-faltivec on G4+?) and help with PowerPC testing, it would be possible to add this feature [to upstream as well].

But as for now, it seems that my patch and/or its context has been misunderstood.

[barracuda156]

@tuffnatty Perhaps fixing it in upstream first will be a better approach, since a) that benefits everyone and not only Macports users, and b) once something is merged to upstream, it will be merged here without any argument.

AFAIK modern GCC can use -faltivec or -maltivec alike, but I am not an expert on SIMD, unfortunately. You could ask Iain here regarding AltiVec with GCC, I guess: https://github.com/iains/darwin-toolchains-start-here/discussions

[kencu]

OK, reopened as misunderstood, for @ryandesign to deal with as he sees fit as port maintainer.

Apologies for responding to @pmetzger 's request to keep the queue clean. I will leave the queue full.

This PR has been sitting here for about a year now, though.

[kencu]

currently, graphite2 does not use the muniversal portgroup, by the way.

https://github.com/macports/macports-ports/blob/master/graphics/graphite2/Portfile

[kencu]

currently, graphite2 does not use the muniversal portgroup, by the way.

https://github.com/macports/macports-ports/blob/master/graphics/graphite2/Portfile

Ah, sorry, I missed that your PR here adds muniversal. My bad.

[tuffnatty]

This PR has been sitting here for about a year now, though.

I am also sorry about that. The upstream maintainers seem to not have reviewed any PRs since December 2022.

[kencu]

@tuffnatty if you wanted to move this along, you might prove/demonstrate that the universal build with your enhancements here works, first of all. Then show that it builds with all SIMD enabled, ideally both on an Intel system building universal, and on an arm system building universal, if you have the hardware available.

With that info in hand and confirmed, committing this would be much easier for someone.

[pmetzger]

Is the benefit of a universal build worth the effort here? Is it going to save enough people enough time over the long run?

[tuffnatty]

@kencu Thank you for your suggestion. What kind of proof would be enough? https://github.com/tuffnatty/sqlitestudio/actions/runs/7202979501/job/19622098015 - this is a GitHub Actions workflow run that uses this patch to build qt5 +universal on Intel GitHub runners. It was not created in order to prove anything, but to have a MacOS universal Qt5 SDK that could be used on GitHub runners. So it does only prove that graphite2 builds +universal with this patch, allowing to build +universal further dependencies. You can also see from the log that the Intel compiler invocations include -mfpmath=sse -msse2 and arm64 don't. Would you like me to disassemble the resulting fat dylib to find autovectorized SIMD instructions in both parts?

% objdump -arch=x86_64 -disassemble /Volumes/SQLiteStudio-3.4.4/SQLiteStudio.app/Contents/Frameworks/libgraphite2.3.dylib \
| awk '/^_.*:$/ {func_found=$1} func_found && /xmm/ {print func_found $0}' \
| head -10
_gr_featureval_clone:    53c5:	0f 57 c0 	xorps	%xmm0, %xmm0
_gr_featureval_clone:    53c8:	0f 11 43 10 	movups	%xmm0, 16(%rbx)
_gr_featureval_clone:    53cc:	0f 11 03 	movups	%xmm0, (%rbx)
_gr_featureval_destroy:    5411:	0f 57 c0 	xorps	%xmm0, %xmm0
_gr_featureval_destroy:    5414:	0f 11 07 	movups	%xmm0, (%rdi)
_gr_make_font:    5921:	0f 57 c9 	xorps	%xmm1, %xmm1
_gr_make_font:    5924:	0f 11 4e 08 	movups	%xmm1, 8(%rsi)
_gr_make_font_with_ops:    5973:	0f 57 c9 	xorps	%xmm1, %xmm1
_gr_make_font_with_ops:    5976:	0f 2e c8 	ucomiss	%xmm0, %xmm1
_gr_make_font_with_ops:    598e:	f3 0f 11 45 dc 	movss	%xmm0, -36(%rbp)
% objdump -arch=arm64 -disassemble /Volumes/SQLiteStudio-3.4.4/SQLiteStudio.app/Contents/Frameworks/libgraphite2.3.dylib \
| awk '/^_.*:$/ {func_found=$1} func_found && /[ \t]v[0-9]/ {print func_found $0}' \
| head -10
_gr_featureval_clone:    6c68:	00 e4 00 6f 	movi.2d	v0, #0000000000000000
_gr_make_font_with_ops:    726c:	08 1c a0 4e 	mov.16b	v8, v0
_gr_make_font_with_ops:    7278:	00 1d a8 4e 	mov.16b	v0, v8
_gr_slot_advance_X:    7abc:	00 1d a8 4e 	mov.16b	v0, v8
_gr_slot_linebreak_before:    8150:	00 e4 00 6f 	movi.2d	v0, #0000000000000000
_gr_slot_linebreak_before:    8294:	80 e7 07 4f 	movi.16b	v0, #252
_gr_slot_linebreak_before:    8298:	02 e4 00 6f 	movi.2d	v2, #0000000000000000
_gr_slot_linebreak_before:    82ac:	83 05 40 0d 	ld1.b	{ v3 }[1], [x12]
_gr_slot_linebreak_before:    82b8:	63 09 40 0d 	ld1.b	{ v3 }[2], [x11]
_gr_slot_linebreak_before:    82bc:	83 0d 40 0d 	ld1.b	{ v3 }[3], [x12]

@pmetzger Thank you, it's probably a valid question. How many saved people should I bring?

[kencu]

So that is a universal build built on an Intel system? Look like. And the Intel SSE is enabled.

What happens when the universal build is done on an arm system?

[tuffnatty]

That's a valid question as well. On a M1/darwin21 system, I see no SSE flags passed in the universal build, which still results in a binary using xmmN registers on x86_64 and vN registers on arm64:

% objdump --disassemble --arch=x86_64 --macho /opt/local/lib/libgraphite2.3.2.1.dylib \   
| awk '/^_.*:$/ {func_found=$1} func_found && /xmm/ {print func_found $0}' \
| head -10
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40bd:	0f 10 06	movups	(%rsi), %xmm0
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40c0:	0f 29 45 c0	movaps	%xmm0, -64(%rbp)
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40e8:	41 0f 10 45 00	movups	(%r13), %xmm0
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40ed:	41 0f 10 4d 10	movups	16(%r13), %xmm1
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40f2:	41 0f 10 55 20	movups	32(%r13), %xmm2
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40f7:	41 0f 10 5d 30	movups	48(%r13), %xmm3
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40fc:	0f 11 00	movups	%xmm0, (%rax)
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    40ff:	0f 11 48 10	movups	%xmm1, 16(%rax)
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    4103:	0f 11 50 20	movups	%xmm2, 32(%rax)
__ZN12_GLOBAL__N_18put_copyERPKhRPiS3_R7regbank:    4107:	0f 11 58 30	movups	%xmm3, 48(%rax)
% objdump --disassemble --arch=arm64 --macho /opt/local/lib/libgraphite2.3.2.1.dylib \
| awk '/^_.*:$/ {func_found=$1} func_found && /[ \t]v[0-9]/ {print func_found $0}' \
| head -10
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    4454:	00 e4 00 6f	movi.2d	v0, #0000000000000000
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    4594:	80 e7 07 4f	movi.16b	v0, #252
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    4598:	02 e4 00 6f	movi.2d	v2, #0000000000000000
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45b0:	83 05 40 0d	ld1.b	{ v3 }[1], [x12]
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45bc:	63 09 40 0d	ld1.b	{ v3 }[2], [x11]
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45c0:	83 0d 40 0d	ld1.b	{ v3 }[3], [x12]
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45cc:	63 11 40 0d	ld1.b	{ v3 }[4], [x11]
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45d0:	83 15 40 0d	ld1.b	{ v3 }[5], [x12]
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45dc:	63 19 40 0d	ld1.b	{ v3 }[6], [x11]
__ZN9graphite22vm7Machine4CodeC2EbPKhS4_htRKNS_4SilfERKNS_4FaceENS_8passtypeEPPh:    45e0:	83 1d 40 0d	ld1.b	{ v3 }[7], [x12]

The symbol names are mangled, but it's probably due to a different XCode version being used.
Building with or without my patch does not change this behavior. That is, on M1/darwin21 the universal build was not broken, while it definitely was on Intel/darwin20.

[kencu]

On an M1 darwin system, does the universal build generate SSE code in the intel portion?

Because that is what I am afraid it won't, and that is what we need to fix if it doesn't.

All SIMD enabled, basically, is what we need. Same build on either Intel or arm.

[kencu]

And yes, this stuff is tricky to get right, I know that. This kind of detail is what slows down commits that we didn't vet ourselves.

[barracuda156]

@kencu

1. Why is having A and B broken preferable to having A fixed and B left as is?

2. What is the point to generate vector instructions which at best will be emulated and possibly won’t run at all?

[kencu]

@kencu

1. Why is having A and B broken preferable to having A fixed and B left as is?
2. What is the point to generate vector instructions which at best will be emulated and possibly won’t run at all?

I am having trouble understanding you.

Can you be more clear what your problem is?

[tuffnatty]

On an M1 darwin system, does the universal build generate SSE code in the intel portion?

Because that is what I am afraid it won't, and that is what we need to fix if it doesn't.

In the comment you were seemingly replying to, I have addressed exactly this question.

[kencu]

On an M1 darwin system, does the universal build generate SSE code in the intel portion?
Because that is what I am afraid it won't, and that is what we need to fix if it doesn't.

In the comment you were seemingly replying to, I have addressed exactly this question.

so no SSE flags are passed in the logs you can see but I can’t, yet you find SSE is still used nonethless?

OK, that is your submission then.

Leave it for Ryan now.

[tuffnatty]

so no SSE flags are passed in the logs you can see but I can’t, yet you find SSE is still used nonethless?

Right.

Regarding the logs I can see but you can't: I can paste the logs here, but would it give you more trust (I am not saying they have any evidence of SSE2 switched on; I am saying, on the opposite, they are missing the flags)? I can tell you how to easily reproduce my findings:

1. On a Apple Silicon system, run sudo port build graphite2 +universal. Check the logs, find no SSE flags therein.
2. Run sudo port install graphite2 +universal. Find xmmN registers (on Intel) and vN registers (on ARM) used in there using the objdump commands above.
3. Apply this commit. Rebuild. Recheck.

[kencu]

OK, I got you.

No SSE flags used, but you find SSE used anyway.

I don’t understand it, but I hear you.

Ryan can decide what he wants to do..

Thanks for your submission.

[tuffnatty]

No SSE flags used, but you find SSE used anyway.

I don’t understand it, but I hear you.

It's very simple. https://godbolt.org/z/xezf5cTsd Here is an example of auto-vectorizable loop. On godbolt.org, I see that passing -Os (the default MacPorts CFLAGS) makes CLang 3.4+ autovectorize it, while for gcc one has to use -O3 or -ftree-vectorize, and -mfpmath=sse -msse2 does not seem to affect this code. Probably we need to come up with a floating-point example.

[tuffnatty]

If I change the ints in the example to floats, the loop is auto-vectorized no matter which flags I pass, even with the oldest versions of Clang and GCC available on https://godbolt.org/z/1eM9j7vzs .

[tuffnatty]

More info. graphite2's documentation:

Floating point maths

On some Intel 32 bit processors, gcc on Linux, will attempt to optimise floating point operations by keeping floating point values in 80 bit (or larger) float registers for as long as possible. In some of the floating point maths performed for collision detection this results in error accumulating which produces different results between 64 bit and 32 bit native code. The current work around is to pass either -ffloat-store (the slow but widely compatible option) or -mfpmath=sse -msse2 (the faster but not as generic), this causes float values to be rounded between every operation or the use of sse float operations which are more strictly specified and therefore more predictable across processor generations. This has not been observed to be an issue on 32 bit MSVC or Mac 32 bit compilers and is not an issue on ARM 32 bit either.

So, I am interpreting this as: the -mfpmath=sse -msse2 only ever made sense on 32-bit Intel gcc/clang. It was not meant to switch SSE auto-vectorization on. It was only intended to keep the rounding errors in the collision detection code similar between 32-bit and 64-bit Intel gcc/clang code.

So, if a universal arm/x86-32 build was a thing, it could be broken by not passing -mfpmath=sse -msse2. Probably this can also affect the universal ppc/x86-32 build being executed on PowerPC. @barracuda156 I can't test it, my G4 is long dead. But even if there is a problem, it only results in small glitches in some shaping details of some esoteric Graphite-enabled fonts on 32-bit powerpc/x86 universal builds.

BTW, the initial bug I was fixing (using ${CMAKE_SYSTEM_PROCESSOR} as the host processor while it seems to mean the build processor in this case) was added to upstream by @ryandesign in an attempt to fix the PowerPC build: silnrsi/graphite@4f78a29 .

[kencu]

if the build is using SSE code when it’s instructed not to (ie SSE is not enabled) that is presumably a build error that needs fixing.

I’m sorry I don’t have more time to deep dive on this in Godbolt, etc. You’ve laid out a good case, thank you. Ryan will be along to assess his port in due course.

[ryandesign]

I have a strong suspicion that @ryandesign does not get notifications about being mentioned in PR threads. I'm unsure why.

Sorry for not responding here for so long. I do get notifications but I don't click on most of them because it has been my experience that often clicking on a notification results in me spending an hour reading lots of comments, having to think about a lot of things I haven't though about in awhile, and then still often not arriving at a solution.

P. S. I am not really sure about MATCHES syntax, but possibly ppc64 should be here too? ${CMAKE_OSX_ARCHITECTURES} MATCHES "arm|ppc"

MATCHES checks if a string matches a regular expression. The regular expression arm|ppc matches any string containing "arm" or "ppc", so ppc64 is already taken care of.

BTW, the initial bug I was fixing (using ${CMAKE_SYSTEM_PROCESSOR} as the host processor while it seems to mean the build processor in this case) was added to upstream by @ryandesign in an attempt to fix the PowerPC build: silnrsi/graphite@4f78a29 .

It was added by me after being suggested by @barracuda156 in https://trac.macports.org/ticket/64532.

I see that @barracuda156 already commented on that commit suggesting the use of CMAKE_OSX_ARCHITECTURES instead 18 months ago.

Have you already read my comment at https://trac.macports.org/ticket/64532#comment:4 where I said CMAKE_SYSTEM_PROCESSOR would be correct for cross compilation if we used CMAKE_TOOLCHAIN_FILE, which we don't, so a good area of investigation would be making MacPorts use a toolchain file?

[ryandesign]

It was added by me after being suggested by @barracuda156 in https://trac.macports.org/ticket/64532.

I'm wrong, it was mentioned by @barracuda156 there but it was added in 88eb463 .

[kencu]

clang should just do the right thing here, but it is not doing so.

perhaps there is a reason it will not smoothly handle the multi-arch build when using those flags.... I think it should... but there may be subtleties going on that I don't know about.

I opened an issue with llvm upstream to see what they think / would recommend for a general fix for this. This issue does arise in other projects too.

llvm/llvm-project#79720

[pmetzger]

It seems to me that although people are interested in this, it's not being worked on actively enough to leave in the queue.

[pmetzger]

I'm closing this for now. It can be re-opened if someone wishes to work on it.