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

gotypes: provide access to component address #149

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mmcloughlin
Copy link
Owner

Extends #148
Fixes #145

@mmcloughlin
Copy link
Owner Author

@klauspost @rleiwang Review would be appreciated if you have time 😄

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2020

Codecov Report

Merging #149 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #149   +/-   ##
=======================================
  Coverage   78.04%   78.04%           
=======================================
  Files          55       55           
  Lines       33395    33395           
=======================================
  Hits        26063    26063           
  Misses       7185     7185           
  Partials      147      147           
Flag Coverage Δ
#integration 8.38% <0.00%> (ø)
#unittests 75.62% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa88270...8618366. Read the comment docs.

@mmcloughlin
Copy link
Owner Author

Oh joy, we seem to have hit a linter error:

tests/fixedbugs/issue145/issue145.s:12:1: [amd64] Shuffle: invalid MOVOU of ret+48(FP); [4]uint32 is 16-byte value

https://github.com/mmcloughlin/avo/pull/149/checks?check_run_id=701016645#step:7:696

@mmcloughlin
Copy link
Owner Author

Filed golang/go#39220. I'll probably just change the test case to a simpler 64-bit return value to avoid this linter problem.

@mmcloughlin
Copy link
Owner Author

Another problem:

[amd64] Halves: invalid MOVQ of ret+8(FP); [2]uint32 is 8-byte value

I can't quite figure out why this is yet. Perhaps it does not handle composite types properly.

Skipping asmdecl linter for this specific test case.

@mmcloughlin
Copy link
Owner Author

@klauspost I have to admit I don't like this very much. I quite like the way errors right now are presented similarly to the way compilers work: multiple errors presented with a file:line reference for each. That's much cleaner than a panic in my opinion.

Let me know if you have ideas on how we could avoid the MustAddr() method without requiring an if err != nil { } block.

The only idea I have is to drop the requirement that Resolve() must refer to a primitive type and then extend the move detection code in Load() and Store():

https://github.com/mmcloughlin/avo/blob/fa88270b07e447bf4d3d65b081c8f728bc4ba1f7/build/zmov.go

This could possibly be adapted to handle larger composite types. I haven't looked at how difficult this would be yet, perhaps the move detection would be harder for composite types. Perhaps there's always a niche use case for Addr() and MustAddr() even if you make Load() and Store() more advanced.

Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Not sure if I would call it more "unsafe" than most assembly code. But it does the job nicely!

@klauspost
Copy link
Contributor

@mmcloughlin I would <3 to have this feature.

Copy link

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way we could get this in, @mmcloughlin ?

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.

gotypes: error with Store(xmm, ReturnIndex(0))
4 participants