On Sun, Oct 11, 2020 at 01:12:40PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > The first attachment fixes the matter you've reported. While confirming that,
> > I observed that gcc builds don't even use the 64-bit code in arch-ppc.h.
> > Oops. The second attachment fixes that.
>
> I reviewed these, and tested the first one on a nearby Apple machine.
> (I lack access to 64-bit PPC, so I can't actually test the second.)
> They look fine, and I confirmed by examining asm output that even
> the rather-old-now gcc version that Apple last shipped for PPC does
> the right thing with the conditionals.
Thanks for reviewing and for mentioning that old-gcc behavior. I had a
comment asserting that gcc 7.2.0 didn't deduce constancy from those
conditionals. Checking again now, it was just $SUBJECT preventing constancy
deduction. I made the patch remove that comment.
> > I plan not to back-patch either of these.
>
> Hmm, I'd argue for a back-patch. The issue of modern compilers
> warning about the incorrect code will apply to all supported branches.
> Moreover, even if we don't use these code paths today, who's to say
> that someone won't back-patch a bug fix that requires them? I do not
> think it's unreasonable to expect these functions to work well in
> all branches that have them.
Okay, I've pushed with a back-patch. compare_exchange-ppc-immediate-v1.patch
affects on code generation are limited to regress.o, so it's quite safe to
back-patch. I just didn't think it was standard to back-patch for the purpose
of removing a -Wsign-compare warning. (Every branch is noisy under
-Wsign-compare.)
atomics-ppc64-gcc-v1.patch does change code generation, in the manner
discussed in the big arch-ppc.h comment (starts with "This mimics gcc").
Still, I've accepted the modest risk.