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.

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

cmd/compile: tighten moves loads into different blocks away from their nilcheck, preventing nilcheck elimination #63726

Open
laboger opened this issue Oct 25, 2023 · 2 comments
Open

cmd/compile: tighten moves loads into different blocks away from their nilcheck, preventing nilcheck elimination #63726

laboger opened this issue Oct 25, 2023 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@laboger
Copy link
Contributor

laboger commented Oct 25, 2023

What version of Go are you using (go version)?

$ go version
tip

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

PPC64

What did you do?

Inspected generated code after CL #531425 to verify expected improvements. The purpose of this change was to avoid ANDCCconst if possible, which removed an instruction that set the flag unnecessarily.

What did you expect to see?

Same or better code than before my change.

What did you see instead?

I noticed that for some functions the NilCheck wasn't optimized, whereas before my change it was. When looking at the GOSSAFUNC output with this change, tighten was able to move a load into a different (successor) block but left the associated NilCheck in the original block. The later NilCheck elimination pass only eliminates the NilCheck if is in the same block as the load or store that uses the NilCheck address.

An example of this is found in internal/abi.(*Name).IsBlank.

Even though I discovered this difference after my change for PPC64, it seems like this is a general problem in tighten if loads or stores are moved to a different block when they had related NilChecks in their original block because then the NilChecks can't be optimized.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 25, 2023
@randall77 randall77 added this to the Unplanned milestone Oct 25, 2023
@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 25, 2023
@dr2chase
Copy link
Contributor

Does the example show the bug even without CL #531425 applied?

@laboger
Copy link
Contributor Author

laboger commented Oct 26, 2023 •
edited

The problem can happen on x86 which should not be affected by CL #531435 (I gave the wrong number above). I see it happens in internal/abi.Name.IsBlank and other internal/abi.Name functions on x86.

If a block contains Nilcheck and Load, and the Load is moved into another block by tighten but leaves the Nilcheck behind, then it can't be eliminated. My change just allowed it to happen in more cases on PPC64 where it was already happening on x86.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@dr2chase @randall77 @gopherbot @laboger