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
cmd/compile: constant folding math/bits functions #21872
Comments
Thank you @markus-oberhumer for the proposal and for offering to work on it, very much appreciated to have contributors jump on board, welcome to the Go project! So as you noticed, in deed that CLA would be necessary to review changes, please sign it if you can. |
Yes, this optimization sounds like a good idea. @odeke-em I think this is too small to be considered as a proposal. Just do it™®. |
Ahaha roger that @randall77 Just do it™® #ShiaLaBeouf. Sure, I'll update the title as this is something that can be done with a CL, and has been accepted. |
I now have my Gerrit account and CLA ready, so I've started working on this optimization. Implementation is indeed straightforward, but I've noticed some suprising issue while adding the test cases: Choosing which functions are considered intrinsics in src/cmd/compile/internal/gc/ssa.go is made under consideration if an arch has efficient runtime instructions for handling that function, so any SSA constant folding is only made for specific archs. For example, I'm just wondering if this is a known issue and if there are any plans of improving this. Thanks! |
(Wrong post - deleted). |
This hasn't really been an issue before. I suppose I would chalk it up to "unfortunate". You might be able to add an additional "argument is a constant" check which triggers intrinsification regardless of arch, but that would be an imperfect check at Node->SSA time. Doesn't seem worth the trouble to fix. |
Ok, thanks, but then unfortunately many archs will be missing the new constant-time optimizations. |
I think all the archs aside from 386 should be able to do all the intrinsics. It just hasn't been implemented yet for some of them. Another option is to always do the intrinsic, and convert back to a call for those archs which can't do the intrinsic. That would allow your optimization to trigger in all cases. That would be a lot of work, though. |
Well, for example math.bits Reverse64() will only be constant-folded for GOARCH=arm64, because no other CPU has a RBIT instruction. But I agree that changing this would be a lot of work (and a much better understanding of the compiler than I have right now). |
How's it going here @markus-oberhumer and @randall77? It's just your friendly Go triager :) |
Seems like an optimization request. Bumping to 1.11. |
We started constant folding bits.TrailingZeros as part of 1.15. |
Actually I did implement this 2 years ago, but I never found the energy to start using Gerrit... In any case the implementation is as simple as outlined in my initial post. And just in case anybody is interested, I've pushed my work (based on the go1.13 branch) to Github - please see https://github.com/markus-oberhumer/golang--go/commits/constant-fold-math-bits--go1.13 and the actual commit at markus-oberhumer-forks@30e05d8 A few test cases are still missing, but otherwise this should be complete. Cheers, |
@markus-oberhumer thanks. Have you signed a CLA? Without that I'm afraid we can't look at your commit. If you have, and you don't mind, I can get your work incorporated, although the commit will be in my name. (I can credit you in the commit message.) Alternatively, the Go project now accepts GitHub PRs... |
Yes, I did sign the CLA back in 2017 and have a Gerrit account. But as mentioned I never found the energy to actually start using Gerrit... |
@josharian Please use whatever parts you find useful from my commit - this is much less work than me starting to rebase the commit to go1.15 and going through the review process. Thanks! |
Aye aye. Can a Googler (@ianlancetaylor maybe) check CLA status for me, just to confirm? Thanks. |
I can send a CL with the fixes and added tests - but CLA might still need to be checked. |
JFTR, my account at https://cla.developers.google.com/clas says: "Google Individual CLA" signed "Sep 14, 2017 03:27 PDT". |
Change https://golang.org/cl/280453 mentions this issue: |
What version of Go are you using (
go version
)?Current git master 5779677
What did you expect to see?
There is a very easy optimization possibility of constant folding various functions from math/bits.
Would there be interest in implementing these?
Personally, I could work on this, but I'm pretty new to the compiler internals and don't have a CLA yet.
The text was updated successfully, but these errors were encountered: