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
