https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83653

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 11 Jan 2018, matthew at wil dot cx wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83653
> 
> --- Comment #5 from Matthew Wilcox <matthew at wil dot cx> ---
> Hi Aldy!
> 
> Thanks for looking into this.  Yes, I agree, there's no way that GCC can know
> this is a constant, but that *should* have been taken care of.  Please pardon
> me copying and pasting from the original source file rather than the
> preprocessed source, but I find it utterly impossible to work with the
> preprocessed source ...
> 
> #define atomic_sub_return(i,v)                                          \
> ({                                                                      \
>         int __ia64_asr_i = (i);                                         \
>         (__builtin_constant_p(i)                                        \
>          && (   (__ia64_asr_i ==   1) || (__ia64_asr_i ==   4)          \
>              || (__ia64_asr_i ==   8) || (__ia64_asr_i ==  16)          \
>              || (__ia64_asr_i ==  -1) || (__ia64_asr_i ==  -4)          \
>              || (__ia64_asr_i ==  -8) || (__ia64_asr_i == -16)))        \
>                 ? ia64_fetch_and_add(-__ia64_asr_i, &(v)->counter)      \
>                 : ia64_atomic_sub(__ia64_asr_i, v);                     \
> })
> 
> That __builtin_constant_p() *should* have led GCC to throw up its hands, not
> bother checking for the +/- 1, 4, 8, 16 cases and just call 
> ia64_atomic_sub(). 
> Looking at the disassembly, I see a BBB bundle, indicating quite strongly to 
> me
> that it is testing for all of these cases, and the __builtin_constant_p is
> being ... ignored?  misunderstood?

The usual events are that we perform some jump threading across
the __builtin_constant_p before it is resolved, exposing the constant
path somewhere 'i' isn't constant.

The builtin really doesn't guarantee what kernel folks want, because
at one point they want it resolved very late so optimization can
eventually figure out 'i' _is_ constant on the other hand some
optimizations might pull out stuff across it and break expectations.

Note I didn't try to figure what exactly goes wrong but I believe
we had duplicate issues (also in the kernel) that were worked
around in the source by just removing the stupid optimization
and relying on GCC to perform it for constant 'i'.

Reply via email to