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

cmd/compile: consider flagging ops that should not be moved between blocks #34950

Open
mundaym opened this issue Oct 17, 2019 · 2 comments

Comments

@mundaym
Copy link
Member

@mundaym mundaym commented Oct 17, 2019

Some intrinsics (such as population count, FMA and rounding) are emitted by the compiler but are not present in the base ISA of the CPU the compiler is targeting. In these cases the compiler emits code to guard the execution of the instruction and fall back to a slower implementation if the required CPU feature is not present.

This technique is currently working fine but I am concerned it might not interact well with block fusing optimizations that we might add in the future (such as those mentioned in #30645) and this could lead to subtle bugs, especially since machines without some CPU features (e.g. SSE 4.1) are fairly rare these days. We already do some optimizations where we perform code movement and speculatively execute code in order to emit conditional select instructions.

I think we should consider marking these ops somehow, perhaps simply with the 'has side effects' flag. This would represent the possibility that these instructions could cause an illegal instruction exception and prevent the compiler from moving them.

The conditional select optimizations special case integer divide ops since they panic if the divisor is 0 for a similar reason: they should not be speculatively executed.

Example:

if cpu.HasPopCount {
    y = PopCount(x)
} else {
    y = GenericPopCount(x)
}

Could be transformed by the compiler into:

y = select(cpu.HasPopCount, PopCount(x), GenericPopCount(x)) 

Currently there is ~zero risk of this transformation occuring because the fall back is generally an expensive function call with side effect. However I think that is the only reason this code transformation wouldn't be applied and that seems a bit fragile.

@mundaym mundaym added this to the Unplanned milestone Oct 17, 2019
@mundaym

This comment has been minimized.

Copy link
Member Author

@mundaym mundaym commented Oct 17, 2019

Note that this would probably need to be a dynamic property of a value for generic ops since whether or not a certain op is in the base ISA differs between architectures. For example, PopCount8 could be guarded on one architecture and unguarded on another.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Oct 17, 2019

All loads fit into this category as well:

if p != nil {
   ... = *p
}

We don't currently have any mechanism to prevent the *p from happening before the if. We're just not doing that lifting optimization yet, so it isn't a problem.

Instead of marking an instruction as immobile, it might be a bit more flexible to mark, when generating the SSA, values that must be scheduled in a block dominated by a particular block b. We often know what b is when generating SSA (it's one of the if branches).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.