Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/23064 )
Change subject: [tablet] avoid -Wpsabi warning in ComputeLowerAndUpperBound on Rocky ARM ...................................................................... Patch Set 4: > > > Hi, could you add some description for the problem instead of > > > calling it. Do I understand that the following happens: > > > > > > In case of older g++, classes with a zero-sized subobject were > > not > > > treated as HFAs. Newer g++ versions have fixed this issue. > > > > > > This could cause problems if we compiled the function into a > > > library > > > with a newer g++, then tried to use it while compiling with an > > > older > > > compiler (or vice versa). Since the new code would use the > > > floating-point SIMD register, this would cause ABI > > incompatibility. > > > > > > This is not the case in KUDU, since it is an internal function, > > so > > > this is just a warning. > > > > > > A custom structure instead of std::pair<double, double> was > > already > > > treated as an HFA with older compilers, so the incompatibility > > > disappears. > > > > > > This commit does not change anything with newer compilers, and > > with > > > older compilers KUDU will probably use a different register to > > pass > > > the return value (so practically nothing changes). > > > > > > ? > > > > I agree with Zoltan here. The patch changes nothing except for > > suppressing the warning. > > Maybe, std::pair is slightly more performant than custom struct > but > > that is not a concern here. However, custom struct certainly adds > > more readability. > > I don’t think std::pair is more performant. On the contrary, the > new structure allows an optimization (using the Vn registers) that > was disabled in older versions of g++ due to a bug (having an empty > base class __pair_base<_T1, _T2> disabled treating it as an HFA), > and is still disabled in newer g++ versions with our current > compiler flags (so the compiled binary remains compatible with > older g++). > > So this change might actually help performance, but it is not even > visible at the ASSEMBLY level. A processor might do something > better if it receives the value in its S0 register. So it’s a > strictly ? 0 gain, but probably == 0, unless the processor can > handle floating-point operations on S0 registers better than on > generic registers (so it’s really architecture-dependent). * strictly >= 0 gain, but probably == 0 -- To view, visit http://gerrit.cloudera.org:8080/23064 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4db585a8eaf152f1fa2884e72e38a4e7c5efa1 Gerrit-Change-Number: 23064 Gerrit-PatchSet: 4 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Wed, 25 Jun 2025 08:42:45 +0000 Gerrit-HasComments: No
