Re: [HACKERS] xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
On Sun, Jul 19, 2015 at 04:32:16PM -0400, Noah Misch wrote: > On Sun, Jul 19, 2015 at 04:22:47PM -0400, Tom Lane wrote: > > More: if the compiler does have a bug like that, how much confidence can > > we have, really, that there are no other miscompiled places and won't be > > any in the future? If we are going to support this configuration, I think > > what the patch ought to do is disable PG_USE_INLINE globally when this > > compiler is detected. That would revert the behavior to what it was > > before 43d89a2. > > That's a reasonable alternative. Here's the patch implementing it. diff --git a/config/test_quiet_include.h b/config/test_quiet_include.h index f4fa4d3..732b231 100644 --- a/config/test_quiet_include.h +++ b/config/test_quiet_include.h @@ -7,3 +7,12 @@ fun() { return 0; } + +/* + * "IBM XL C/C++ for AIX, V12.1" miscompiles, for 32-bit, some inline + * expansions of ginCompareItemPointers() "long long" arithmetic. To take + * advantage of inlining, build a 64-bit PostgreSQL. + */ +#if defined(__ILP32__) && defined(__IBMC__) +#error "known inlining bug" +#endif -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
On Sun, Jul 19, 2015 at 04:22:47PM -0400, Tom Lane wrote: > Andres Freund writes: > > On July 19, 2015 9:50:33 PM GMT+02:00, Noah Misch wrote: > >> I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to > >> exclude xlc 32-bit configurations. The last 32-bit AIX kernel exited > >> support on 2012-04-30. > > > I vote to simply error out in that case then. Trying to fix individual > > compiler bugs in an niche OS sounds like a bad idea. Fair principle. PostgreSQL 9.4.4 does work in this configuration due to the accident of -qlonglong causing a warning that in turn disables PG_USE_INLINE. I do not want 9.4.5 to break this configuration, and I also don't want to restore the works-accidentally state. Adding a few more conditions to one #if is cheap and has neither problem. > I think I'm with Andres --- are there really enough users of this > configuration to justify working around such a bug? We never have the benefit of an answer to that question. > More: if the compiler does have a bug like that, how much confidence can > we have, really, that there are no other miscompiled places and won't be > any in the future? If we are going to support this configuration, I think > what the patch ought to do is disable PG_USE_INLINE globally when this > compiler is detected. That would revert the behavior to what it was > before 43d89a2. That's a reasonable alternative. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
Andres Freund writes: > On July 19, 2015 9:50:33 PM GMT+02:00, Noah Misch wrote: >> I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to >> exclude xlc 32-bit configurations. The last 32-bit AIX kernel exited >> support on 2012-04-30. > I vote to simply error out in that case then. Trying to fix individual > compiler bugs in an niche OS sounds like a bad idea. I think I'm with Andres --- are there really enough users of this configuration to justify working around such a bug? More: if the compiler does have a bug like that, how much confidence can we have, really, that there are no other miscompiled places and won't be any in the future? If we are going to support this configuration, I think what the patch ought to do is disable PG_USE_INLINE globally when this compiler is detected. That would revert the behavior to what it was before 43d89a2. We have absolutely no field experience justifying the assumption that a narrower fix would be safe. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
On July 19, 2015 9:50:33 PM GMT+02:00, Noah Misch wrote: >In a 32-bit build, xlc 12.1 for AIX miscompiles three inline expansions >of >ginCompareItemPointers(), all in ginget.c (line numbers per commit >9aa6634): > >739Assert(!ItemPointerIsValid(&entry->curItem) || >740 ginCompareItemPointers(&entry->curItem, &advancePast) <= 0); > >847} while (ginCompareItemPointers(&entry->curItem, &advancePast) ><= >0); > >915if (ginCompareItemPointers(&key->curItem, &advancePast) > 0) > >For one of the arguments, instead of computing > hi << 32 | lo << 16 | posid >it computes > (lo << 16) << 32 | lo << 16 | posid > >PostgreSQL 9.4, which introduced the inline ginCompareItemPointers(), >is the >oldest version affected. The problem remained invisible until my >recent >commit 43d89a2; the quiet inline "configure" test had been failing, >leaving >PG_USE_INLINE unset. > >I tried some workarounds. Introducing intermediate variables or moving >parts >of the calculation down into other inline functions did not help. The >code >compiles fine when not inlined. Changing "hi << 32" to "hi << 33" >worked (we >need just 48 of the 64 bits), but it presumably reduces performance on >ABIs >where the current bit shifts boil down to a no-op. > >I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to >exclude >xlc 32-bit configurations. The last 32-bit AIX kernel exited support >on >2012-04-30. I vote to simply error out in that case then. Trying to fix individual compiler bugs in an niche OS sounds like a bad idea. Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
In a 32-bit build, xlc 12.1 for AIX miscompiles three inline expansions of ginCompareItemPointers(), all in ginget.c (line numbers per commit 9aa6634): 739 Assert(!ItemPointerIsValid(&entry->curItem) || 740ginCompareItemPointers(&entry->curItem, &advancePast) <= 0); 847 } while (ginCompareItemPointers(&entry->curItem, &advancePast) <= 0); 915 if (ginCompareItemPointers(&key->curItem, &advancePast) > 0) For one of the arguments, instead of computing hi << 32 | lo << 16 | posid it computes (lo << 16) << 32 | lo << 16 | posid PostgreSQL 9.4, which introduced the inline ginCompareItemPointers(), is the oldest version affected. The problem remained invisible until my recent commit 43d89a2; the quiet inline "configure" test had been failing, leaving PG_USE_INLINE unset. I tried some workarounds. Introducing intermediate variables or moving parts of the calculation down into other inline functions did not help. The code compiles fine when not inlined. Changing "hi << 32" to "hi << 33" worked (we need just 48 of the 64 bits), but it presumably reduces performance on ABIs where the current bit shifts boil down to a no-op. I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to exclude xlc 32-bit configurations. The last 32-bit AIX kernel exited support on 2012-04-30. There's little reason to prefer 32-bit PostgreSQL under a 64-bit kernel, so that configuration can do without the latest GIN optimization. One alternative would be to distill a configure-time test detecting the bug, so fixed xlc versions reacquire the optimization. Another alternative is to change the bit layout within the uint64; for example, we could use the most-significant 48 bits instead of the least-significant 48 bits. I dislike the idea of a niche configuration driving that choice. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers