| Message ID | b603d1dfc5122930728b042184295c490a35675c.camel@vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000,gcc-8] Improve handling of built-in initialization. [PR95952] | expand |
Hi! On Tue, Jul 14, 2020 at 12:15:01PM -0500, will schmidt wrote: > We've got a scenario with a combination of old hardware, gcc-8 and > binutils where gcc will ICE during it's selftest. This ICE was exposed > when the builtin processing for better #pragma support was added, where > we no longer skip builtin initialization based on the current mask. > OK for gcc-8 ? Yes, but some formatting nits: > + /* PR95952: Gracefully skip builtins that do not have the icode properly > + set, but do have the builtin mask set. This has occurred in older gcc > + builds with older binutils support when binutils refuses code generation > + for instructions that it does not support. This was exposed by changes > + allowing all builtins being initialized for better #pragma support. */ Nice useful comment :-) > + if (d->icode == CODE_FOR_nothing && d->mask) { > + HOST_WIDE_INT builtin_mask = rs6000_builtin_mask; The { goes on the next line: if (d->icode == CODE_FOR_nothing && d->mask) { HOST_WIDE_INT builtin_mask = rs6000_builtin_mask; (two spaces indent, twice). if (TARGET_DEBUG_BUILTIN) { fprintf (stderr, "altivec_init_builtins, altivec predicate builtin %s", d->name); fprintf (stderr, " was skipped. icode:%d, mask: %lx, builtin_mask: 0x%lx", d->icode, d->mask, builtin_mask); (those lines are much too long, but debug code, I can't say I care much). } continue; } So: { always goes on a line of its own, two columns extra indent both before and after it; } always aligns exactly with the {. Okay for GCC 8 with that cleaned up. Thank you! Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 75d40367a98..18713599d3b 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -18015,10 +18015,24 @@ altivec_init_builtins (void) if (rs6000_overloaded_builtin_p (d->code)) mode1 = VOIDmode; else { + /* PR95952: Gracefully skip builtins that do not have the icode properly + set, but do have the builtin mask set. This has occurred in older gcc + builds with older binutils support when binutils refuses code generation + for instructions that it does not support. This was exposed by changes + allowing all builtins being initialized for better #pragma support. */ + if (d->icode == CODE_FOR_nothing && d->mask) { + HOST_WIDE_INT builtin_mask = rs6000_builtin_mask; + if (TARGET_DEBUG_BUILTIN) { + fprintf (stderr, "altivec_init_builtins, altivec predicate builtin %s", d->name); + fprintf (stderr, " was skipped. icode:%d, mask: %lx, builtin_mask: 0x%lx", + d->icode, d->mask, builtin_mask); + } + continue; + } /* Cannot define builtin if the instruction is disabled. */ gcc_assert (d->icode != CODE_FOR_nothing); mode1 = insn_data[d->icode].operand[1].mode; }
Hi, We've got a scenario with a combination of old hardware, gcc-8 and binutils where gcc will ICE during it's selftest. This ICE was exposed when the builtin processing for better #pragma support was added, where we no longer skip builtin initialization based on the current mask. Per the bug report and assorted debug, the ICE occurrs when building the gcc-8 branch on a 970* based system with an old binutils. (gcc-9 and newer is OK. binutils 2.34 is reported to allow success). The attached patch adds a clause to the builtin initialization to skip initialization of a builtin when the builtin mask is set but the icode value is zero. The subsequent assert check remains in place. I've successfully tested this on a yellowdog (970mp) based system. Mikael has confirmed this allows success on his system. OK for gcc-8 ? Thanks, -Will 2020-07-13 Will Schmidt <will_schmidt@vnet.ibm.com> gcc/ChangeLog: PR target/95952 * gcc/config/rs6000.c (altivec_init_builtins): Add continue clause to predicate builtin handling.