8000 BUG: simplify code remove strict aliasing violation by juliantaylor · Pull Request #6252 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: simplify code remove strict aliasing violation #6252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 26, 2015
Merged

BUG: simplify code remove strict aliasing violation #6252

merged 1 commit into from
Aug 26, 2015

Conversation

juliantaylor
Copy link
Contributor

Didn't think the violation could cause issues but apparently some
compilers do mess it up. Also the old code is overly complicated, don't
know what I was thinking ...

Closes gh-6251

@juliantaylor
BUG: simplify code remove strict aliasing violation
a62abc8 
Didn't think the violation could cause issues but apparently some
compilers do mess it up. Also the old code is overly complicated, don't
know what I was thinking ...

Closes numpygh-6251
jaimefrio added a commit that referenced this pull request Aug 26, 2015
@jaimefrio
Merge pull request #6252 from juliantaylor/comp-alias
16dadaf 
BUG: simplify code remove strict aliasing violation
@jaimefrio jaimefrio merged commit 16dadaf into numpy:master Aug 26, 2015
@jaimefrio
Copy link
Member

Thanks Julian.

@jaimefrio
Copy link
Member

This should probably be backported, @charris.

@pv
Copy link
Member
pv commented Aug 26, 2015

So, to clarify --- (npy_uint32)&tmp and &tmp breaks strict-aliasing?
EDIT indeed, the former does...

@juliantaylor
Copy link
Contributor Author

yes interpreting anything that aliases as a different type (except union, char and compatible types (e.g. signed/unsigned)) violates the rule. Here interpreting a double as a integer to perform the bitwise and

so gcc 4.3.4 decided to produce this:

mov    0x18(%rsp),%rax
cmpeqsd %xmm1,%xmm0
and    $0x1,%eax
movlpd %xmm0,0x18(%rsp)
mov    %al,(%r15)

it completely ignores the comparison result which it is technically allowed as the result stored by the comparison result cannot overlap the temporary variable we stored it in. Intended was that it reloads the variable back into rax for the and.

juliantaylor added a commit that referenced this pull request Aug 26, 2015
@juliantaylor
Merge pull request #6252 from juliantaylor/comp-alias
b49adda 
BUG: simplify code remove strict aliasing violation
@juliantaylor
Copy link
Contributor Author

@charris, I have merged into maintenance branch (it was rebased)

@juliantaylor juliantaylor deleted the comp-alias branch August 26, 2015 21:45
@charris
Copy link
Member
charris commented Aug 26, 2015

Great, thanks Julian.

@juliantaylor
Copy link
Contributor Author

fyi 4.9 produces what I expected, it removes the temporary variable and thus removes the strict aliasing violation:

cmpeqsd %xmm1,%xmm0
movapd %xmm0,%xmm6
movq   %xmm6,%rdi << dump it directly into the GPR
and    $0x1,%edi
mov    %dil,(%rcx)

the new way is a bit more inefficient but it doesn't matter as its just a peeling loop.

@cgohlke cgohlke mentioned this pull request Oct 8, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juliantaylor @jaimefrio @pv @charris
0