fails to build on on ppc64el

[spaetz]

Hi,
s2geometry is in Debian, great. However, it fails to build on 2 architectures, and I have no clue what could be wrong, or how to fix it. (This is tag/v0.10.0, BTW)

The Debian issue is at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1023961 pointing to the build protocol at:
https://buildd.debian.org/status/fetch.php?pkg=s2geometry&arch=ppc64el&ver=0.10.0-2&stamp=1667081576&raw=0

The relevant bit seems to be:

/<<PKGBUILDDIR>>/src/s2/s2edge_crossings.cc: In instantiation of ‘bool S2::internal::GetStableCrossProd(const Vector3<T>&, const Vector3<T>&, Vector3<T>*) [with T = long double]’:
/<<PKGBUILDDIR>>/src/s2/s2edge_crossings.cc:127:54:   required from here
/<<PKGBUILDDIR>>/src/s2/s2edge_crossings.cc:115:31: error: ‘(6.15348059642740421245081038903225e-15l / 5.40431955284459475358983848622456e+16l)’ is not a constant expression
  115 |       (32 * kSqrt3 * DBL_ERR) /
      |       ~~~~~~~~~~~~~~~~~~~~~~~~^
  116 |       (kRobustCrossProdError.radians() / T_ERR - (1 + 2 * kSqrt3));
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/<<PKGBUILDDIR>>/src/s2/s2edge_crossings.cc: In instantiation of ‘bool S2::GetIntersectionSimple(const Vector3<T>&, const Vector3<T>&, const Vector3<T>&, const Vector3<T>&, Vector3<T>*) [with T = long double]’:
/<<PKGBUILDDIR>>/src/s2/s2edge_crossings.cc:485:28:   required from here
/<<PKGBUILDDIR>>/src/s2/s2edge_crossings.cc:458:10: error: ‘(1.2e+1l / 7.20575940379279305358983848622456e+16l)’ is not a constant expression
  458 |       12 / (kIntersectionError.radians() / T_ERR - (2 + 2 * kSqrt3));
      |       ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[3]: *** [CMakeFiles/s2.dir/build.make:653: CMakeFiles/s2.dir/src/s2/s2edge_crossings.cc.o] Error 1

[spaetz]

‘(6.15348059642740421245081038903225e-15l / 5.40431955284459475358983848622456e+16l)’ is not a constant expression

?

[spaetz]

Second issue on s390x is a build failure that was fixed 6 months ago in s2geometry by: 7a40135
so that one is fine and fixed.

[jmr]

This looks like a compiler bug or quality-of-implementation issue. From a bit of playing with godbolt, long double constexprs don't work with * or /, but work with + and -.

https://godbolt.org/z/WYz5fY538

This works fine on everything but PPC that I tried. It also works with PPC clang, but not PPC gcc. Could you report this to gcc and see what they say?

For reference, here's the real code:

s2geometry/src/s2/s2edge_crossings.cc

Line 115 in 2ff8244

constexpr T kMinNorm =

[spaetz]

Thanks for the feedback. I agree with you assessment. For now, we have removed the constexpr optimization on ppc to make it compile. I'll folloow up with gcc.

…

-------- Ursprüngliche Nachricht -------- Von: Jesse Rosenstock ***@***.***> Gesendet: 16. November 2022 08:48:42 MEZ An: google/s2geometry ***@***.***> CC: Sebastian Spaeth ***@***.***>, Author ***@***.***> Betreff: Re: [google/s2geometry] fails to build on on ppc64el (Issue #279) This looks like a compiler bug or quality-of-implementation issue. From a bit of playing with godbolt, `long double` `constexpr`s don't work with `*` or `/`, but work with `+` and `-`. https://godbolt.org/z/WYz5fY538 This works fine on everything but PPC that I tried. It also works with PPC clang, but not PPC gcc. Could you report this to gcc and see what they say? For reference, here's the real code: https://github.com/google/s2geometry/blob/2ff824474f0c4dfb157a0d056e4a6bb76bfa690f/src/s2/s2edge_crossings.cc#L115

-- Reply to this email directly or view it on GitHub: #279 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***> -- Sent from mobile device.

[spaetz]

Just for completeness' sake: Upstream bug filed at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107745

Let's see what gcc makes of it.
(and sorry, I left your "I" in there, making it seem as if I did all the analysis, I don't intent to take your credit)

UPDATE: and has been resolved duplicate of a 17 year old bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19779

[jmr]

Just wondering, does clang support this architecture, and does the code compile without workarounds with it? @spaetz

[spaetz]

Sorry, I don't even own that architecture, just reporting on what the Debian build system does which attempts to build on basically all arches. I know next to nothing about clang, I am embarrassed to admit.

[barracuda156]

@jmr Until/unless GCC addresses that, maybe we could make a fix in the master?

[jmr]

Yes, we can do that since it's an easy work-around. Could you file another bug with gcc and make it clear that this is fails-to-compile-valid, not missed-optimization.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107745#c4 mentions "powerpc64le-linux is phasing that out and switching to IEEE quad instead". Are you seeing this on Linux, OS X Snow Leopard (~10 years past EOL), or something else?

[barracuda156]

@jmr

Yes, we can do that since it's an easy work-around. Could you file another bug with gcc and make it clear that this is fails-to-compile-valid, not missed-optimization.

The original ticket tracks the matter, and had some responses today: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19779

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107745#c4 mentions "powerpc64le-linux is phasing that out and switching to IEEE quad instead". Are you seeing this on Linux, OS X Snow Leopard (~10 years past EOL), or something else?

I get this on OS X, yeah. But the same gonna be the case on OpenBSD, for example, etc.

[jmr]

Worked around the gcc bug in 84bfd2c.

[barracuda156]

Worked around the gcc bug in 84bfd2c.

@jmr Wow, that’s a massive commit! Thank you, I will try it.

BTW, are we to expect any changes for Big-endian (for better or worse)?

[jmr]

See #316 (comment) for BE status. No change expected.