This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][AArch64] Implement new intrinsics vabsd_s64 and vnegd_s64


On Tue, Aug 28, 2018 at 03:59:25AM -0500, Vlad Lazar wrote:
> Gentle ping.
> 
> On 08/08/18 17:38, Vlad Lazar wrote:
> > On 01/08/18 18:35, James Greenhalgh wrote:
> >> On Wed, Aug 01, 2018 at 07:13:53AM -0500, Vlad Lazar wrote:
> >>> On 31/07/18 22:48, James Greenhalgh wrote:
> >>>> On Fri, Jul 20, 2018 at 04:37:34AM -0500, Vlad Lazar wrote:
> >>>>> Hi,
> >>>>>
> >>>>> The patch adds implementations for the NEON intrinsics vabsd_s64 and vnegd_s64.
> >>>>> (https://developer.arm.com/products/architecture/cpu-architecture/a-profile/docs/ihi0073/latest/arm-neon-intrinsics-reference-architecture-specification)
> >>>>>
> >>>>> Bootstrapped and regtested on aarch64-none-linux-gnu and there are no regressions.
> >>>>>
> >>>>> OK for trunk?
> >>>>>
> >>>>> +__extension__ extern __inline int64_t
> >>>>> +__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
> >>>>> +vnegd_s64 (int64_t __a)
> >>>>> +{
> >>>>> +  return -__a;
> >>>>> +}
> >>>>
> >>>> Does this give the correct behaviour for the minimum value of int64_t? That
> >>>> would be undefined behaviour in C, but well-defined under ACLE.
> >>>>
> >>>> Thanks,
> >>>> James
> >>>>
> >>>
> >>> Hi. Thanks for the review.
> >>>
> >>> For the minimum value of int64_t it behaves as the ACLE specifies:
> >>> "The negative of the minimum (signed) value is itself."
> >>
> >> What should happen in this testcase? The spoiler is below, but try to work out
> >> what should happen and what goes wrong with your implementation.
> >>
> >>    int foo (int64_t x)
> >>    {
> >>      if (x < (int64_t) 0)
> >>        return vnegd_s64(x) < (int64_t) 0;
> >>      else
> >>        return 0;
> >>    }
> >>    int bar (void)
> >>    {
> >>      return foo (INT64_MIN);
> >>    }
> >> Thanks,
> >> James
> >>
> >>
> >> -----
> >>
> >> <spoiler!>
> >>
> >>
> >>
> >>
> >> INT64_MIN < 0 should be true, so we should return vnegd_s64(INT64_MIN) < 0.
> >> vnegd_s64(INT64_MIN) is identity, so the return value should be
> >> INT64_MIN < 0; i.e. True.
> >>
> >> This isn't what the compiler thinks... The compiler makes use of the fact
> >> that -INT64_MIN is undefined behaviour in C, and doesn't need to be considered
> >> as a special case. The if statement gives you a range reduction to [-INF, -1],
> >> negating that gives you a range [1, INF], and [1, INF] is never less than 0,
> >> so the compiler folds the function to return false. We have a mismatch in
> >> semantics
> >>
> > I see your point now. I have updated the vnegd_s64 intrinsic to convert to
> > unsigned before negating. This means that if the predicted range of x is
> > [INT64_MIN, y], then the predicted range of vnegd_s64 (x) will be
> > ~[INT64_MIN + 1, y] which seems to resolve the issue. I've also added testcases
> > which reflect the issue you've pointed out. Note that I've change the vabsd_s64
> > intrinsic in order to avoid moves between integer and vector registers.

I think from my reading of the standard that this is OK, but I may be rusty
and missing a corner case.

OK for trunk.

Thanks,
James


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]