Skip to content
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.

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

Fix unannounced modification of input operand 8 (lda4) in Haswell GEMVN microkernel #2019

Merged
merged 2 commits into from
Feb 15, 2019
Merged

Conversation

martin-frbg
Copy link
Collaborator

Fixes miscompilation with gcc9 -ftree-vectorize (related to issue #2009 and PR #2018)

Fixes miscompilation with gcc9 -ftree-vectorize (related to issue #2009)
@bartoldeman
Copy link
Contributor

Another way to do this is to rename. Swapping %2 (x) with %8 and then adding the + for 2 gives a minimal diff, but you could also rename 2 to 3, 3 to 4, .. 7 to 8 and then 8 to 2.

@martin-frbg martin-frbg changed the title Save and restore input argument 8 (lda4) Fix unannounced modification of input operand 8 (lda4) in Haswell GEMVN microkernel Feb 15, 2019
@bartoldeman
Copy link
Contributor

That looks good to me. An alternative that avoids this awkward reordering is to use "asmSymbolicName"s, as documented here: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
Very old GCCs did not have those: I was digging a bit and found those were added in GCC 3.1 (from 2002).

@bartoldeman
Copy link
Contributor

In any case this becomes a moot point once it's rewritten to use intrinsics like the skylake-x code, so for now I understand it's just about fixing bugs.

@martin-frbg martin-frbg added this to the 0.3.6 milestone Feb 15, 2019
@martin-frbg
Copy link
Collaborator Author

Thanks again for the review and additional pointers. I'll probably go with the reordering for now, although it is a bit uglier with the next file (the dtrsm_kernel_RN_haswell mentioned in #2009).
Similar bugs exist in SGEMVN for Nehalem and Sandybridge, and in some GEMVN and TRSM
kernels for Bulldozer and Piledriver, I'll see if I can get them fixed over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants