[go] cmd/compile: treat singly-assigned func vars as static in escape analysis
Jake Bailey (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- David Chase
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- David Chase
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
There is a trick we can use here: a nil func cannot be called, becauseFYI we use basically the same trick in the devirtualizer already.
Related details
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
// and assigned exactly once, we can use thatIs that `exactly once` actually a problem? If we find all the assignments, then we can just analyze all of these functions that were assigned, right?
That could solve #73132
Related details
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
// and assigned exactly once, we can use thatIs that `exactly once` actually a problem? If we find all the assignments, then we can just analyze all of these functions that were assigned, right?
That could solve #73132
That's actually something I thought about after sending it, I just wasn't sure if there was much real world use. I didn't realize there was an issue for it. I'll look into it!
Related details
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
// and assigned exactly once, we can use thatIs that `exactly once` actually a problem? If we find all the assignments, then we can just analyze all of these functions that were assigned, right?
That could solve #73132
FYI: I once did a POC for signature based optimizations for unexported interface method calls and it worked, but its been a while so i don't remember the details.
https://go-review.googlesource.com/c/go/+/771280/2/src/cmd/compile/internal/escape/call.go#36
Related details
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
// and assigned exactly once, we can use thatMateusz PoliwczakIs that `exactly once` actually a problem? If we find all the assignments, then we can just analyze all of these functions that were assigned, right?
That could solve #73132
FYI: I once did a POC for signature based optimizations for unexported interface method calls and it worked, but its been a while so i don't remember the details.
https://go-review.googlesource.com/c/go/+/771280/2/src/cmd/compile/internal/escape/call.go#36
And that opt used similar idea.
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
This optimization fires 7 times in std, 100 times in cmd, and 51And how many in x/tools? We use this a lot! :)
Jake Bailey (Gerrit)
Jake Bailey uploaded new patchset![]( "Open in Gerrit")
Related details
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
This optimization fires 7 times in std, 100 times in cmd, and 51And how many in x/tools? We use this a lot! :)
Wow, >750 times.
However, my numbers are not perfect; this only counts how many times the new code is able to actually use the func for escape analysis. It doesn't count when that actually did anything. I'm trying to gather those numbers now.
Related details
- Alan Donovan
- David Chase
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
This optimization fires 7 times in std, 100 times in cmd, and 51Jake BaileyAnd how many in x/tools? We use this a lot! :)
Wow, >750 times.
However, my numbers are not perfect; this only counts how many times the new code is able to actually use the func for escape analysis. It doesn't count when that actually did anything. I'm trying to gather those numbers now.
Wow, >750 times.
Thanks. We do use it a lot! Happy to provide coverage. ;)
Related details
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey uploaded new patchset![]( "Open in Gerrit")
Related details
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jake Bailey
| Commit-Queue | +1 |
1 comment
This optimization fires 7 times in std, 100 times in cmd, and 51Jake BaileyAnd how many in x/tools? We use this a lot! :)
Alan DonovanWow, >750 times.
However, my numbers are not perfect; this only counts how many times the new code is able to actually use the func for escape analysis. It doesn't count when that actually did anything. I'm trying to gather those numbers now.
Wow, >750 times.
Thanks. We do use it a lot! Happy to provide coverage. ;)
Unfortunately, it doesn't seem like very many allocs actually get eliminated. I'm looking into why I find so many funcs but they don't seem to impact much.
Related details
- Alan Donovan
- David Chase
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jake Bailey
| Commit-Queue | +1 |
1 comment
// and assigned exactly once, we can use thatMateusz PoliwczakIs that `exactly once` actually a problem? If we find all the assignments, then we can just analyze all of these functions that were assigned, right?
That could solve #73132
Mateusz PoliwczakFYI: I once did a POC for signature based optimizations for unexported interface method calls and it worked, but its been a while so i don't remember the details.
https://go-review.googlesource.com/c/go/+/771280/2/src/cmd/compile/internal/escape/call.go#36
And that opt used similar idea.
Yep, it's totally possible to generalize this: https://go-review.googlesource.com/c/go/+/771500
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 5 comments![]( "Open in Gerrit")
// non-nil RHS at the same function level (not inside a nested closure).Inside a loop is ok?
(i.e. this is Static Single Assignment.)
closureDepth--You could do this with a `defer`, then you don't need the `abort` flag. Just `return Any(n.Func, do)`.
if Any(name.Curfn, do) {This is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
if !ok || as.Y == nil {as.Y == nil should return true? We use that for zero-initialization.
for y.Op() == OCONVNOP {
y = y.(*ConvExpr).X
}Maybe this should be inside `IsZero`?
(not necessary for this CL, just curious.)
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
// and assigned exactly once, we can use thatMateusz PoliwczakIs that `exactly once` actually a problem? If we find all the assignments, then we can just analyze all of these functions that were assigned, right?
That could solve #73132
Mateusz PoliwczakFYI: I once did a POC for signature based optimizations for unexported interface method calls and it worked, but its been a while so i don't remember the details.
https://go-review.googlesource.com/c/go/+/771280/2/src/cmd/compile/internal/escape/call.go#36
Jake BaileyAnd that opt used similar idea.
Yep, it's totally possible to generalize this: https://go-review.googlesource.com/c/go/+/771500
- Alan Donovan
- David Chase
- Jake Bailey
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 2 comments![]( "Open in Gerrit")
// assignments, has any complex assignments (OAS2, OASOP, ORANGE), or
// is assigned inside a closure.Is there any reason behind that, other than reducing the complexity? Just curious.
func SingleAssignment(name *Name) *AssignStmt {Why this returns `AssignStmt` instead of a `Node`? This could allow handling of `OAS2` and such easier.
Maybe renaming this to `StaticValueSkipZeroVal` would be better?
Jake Bailey (Gerrit)
Jake Bailey uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 3 comments![]( "Open in Gerrit")
// non-nil RHS at the same function level (not inside a nested closure).Inside a loop is ok?
(i.e. this is Static Single Assignment.)
Inside a loop is fine; really, any assignment is fine since we're not determining a static value, just what the escape analyzer sees.
// assignments, has any complex assignments (OAS2, OASOP, ORANGE), or
// is assigned inside a closure.Is there any reason behind that, other than reducing the complexity? Just curious.
Largely it was to limit scope; I don't think the existing StaticValue handled that so what I stuck in this CL didn't either.
if Any(name.Curfn, do) {This is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
That's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
// assignments, has any complex assignments (OAS2, OASOP, ORANGE), or
// is assigned inside a closure.Jake BaileyIs there any reason behind that, other than reducing the complexity? Just curious.
Largely it was to limit scope; I don't think the existing StaticValue handled that so what I stuck in this CL didn't either.
I should eliminate this condition, yeah.
I'm sort of thinking about pulling CL 771500 backward in the stack, though, as that is the more general change. The problem is just that we don't do the whole oracle thing yet and that's what the rest of my stack is trying to do.
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 2 comments![]( "Open in Gerrit")
// assignments, has any complex assignments (OAS2, OASOP, ORANGE), or
// is assigned inside a closure.Jake BaileyIs there any reason behind that, other than reducing the complexity? Just curious.
Largely it was to limit scope; I don't think the existing StaticValue handled that so what I stuck in this CL didn't either.
Hmm, I think it does since it follows `OCLOSURE`?
I just wonder whether removing the `closureDepth > 0` is just enough to support this.
StaticValue skips for non-PAUTO, should we do that also here? See L902 here.
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
FYI you can take a look at `devirtualize.go` as we do similar things there, i.e. the `analyze` func.
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
So in general, this CL contains the "native" / minimal version of this change, but to do more, I think I'd really want to have switched over to the ReassignOracle to track these assignments. thepudds had told me that this will break things that aren't tested, though, so I'm not sure exactly how you all want me to do it.
I certainly prefer the method in my last CL in this stack where I can merge the results from all assinged funcs, rather than special casing a single func.
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
So in general, this CL contains the "native" / minimal version of this change, but to do more, I think I'd really want to have switched over to the ReassignOracle to track these assignments. thepudds had told me that this will break things that aren't tested, though, so I'm not sure exactly how you all want me to do it.
I certainly prefer the method in my last CL in this stack where I can merge the results from all assinged funcs, rather than special casing a single func.
Personally I think we might need a specialized oracle just for this usecase. I have done that for devirtualizer and it seems to serve us well.
I think that we also have a low-hanging fruit in the escape analysis i.e. to track all possible types of interface variables, that then we can use the same trick as in the CL 771500 to improve escape analysis results. And that also seems like a specialized need (although seems like a re-use of devirtualizer logic, with tiny modifications)
But i will defer that to @k...@golang.org.
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
v := ir.StaticValue(call.Fun)If we called `FuncSingleAssignment` here instead of `StaticValue`, shouldn't that work?
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
switch n.Op() {I think we need to handle `OSWITCH`, for example:
```
foo := any(func(a *int) {escape = a})
switch foo := foo.(type) {
case func(a *int):
foo = func(a *int) {}
foo(new(int)) // might not escape
}
```
I think it might miss the implicit var declaration.
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
Mateusz PoliwczakSo in general, this CL contains the "native" / minimal version of this change, but to do more, I think I'd really want to have switched over to the ReassignOracle to track these assignments. thepudds had told me that this will break things that aren't tested, though, so I'm not sure exactly how you all want me to do it.
I certainly prefer the method in my last CL in this stack where I can merge the results from all assinged funcs, rather than special casing a single func.
Personally I think we might need a specialized oracle just for this usecase. I have done that for devirtualizer and it seems to serve us well.
I think that we also have a low-hanging fruit in the escape analysis i.e. to track all possible types of interface variables, that then we can use the same trick as in the CL 771500 to improve escape analysis results. And that also seems like a specialized need (although seems like a re-use of devirtualizer logic, with tiny modifications)
But i will defer that to @k...@golang.org.
I think that's right, frankly. I'm going to kill the stack and just turn this CL into one with a new oracle, and actually handle the multi-assignment case. I think that will basically resolve all of the comments.
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
if Any(name.Curfn, do) {Jake BaileyThis is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
That's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
But on the other hand, without this CL the `StaticValue` is used, which has the same running time, right? So maybe we don't need a oracle at all?
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
if Any(name.Curfn, do) {Jake BaileyThis is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
Mateusz PoliwczakThat's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
But on the other hand, without this CL the `StaticValue` is used, which has the same running time, right? So maybe we don't need a oracle at all?
My point now is what if we only swapped that `StaticValue` call with a specialized `StaticValue` call that handles multiple-assignments? Maybe that is all we need?
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
if Any(name.Curfn, do) {Jake BaileyThis is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
Mateusz PoliwczakThat's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
But on the other hand, without this CL the `StaticValue` is used, which has the same running time, right? So maybe we don't need a oracle at all?
Yes, and the current version of this stack also only does its collection for func-typed nodes, which does limit things.
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
if Any(name.Curfn, do) {Jake BaileyThis is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
Mateusz PoliwczakThat's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
Jake BaileyBut on the other hand, without this CL the `StaticValue` is used, which has the same running time, right? So maybe we don't need a oracle at all?
Yes, and the current version of this stack also only does its collection for func-typed nodes, which does limit things.
Oh, I see what you mean. Yeah, though it isn't much work to encapsulate it into an "oracle" struct and save it in the rare case it's used. I'm making a new bespoke visitor now just to see and it's turning out fine so far...
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
if Any(name.Curfn, do) {Jake BaileyThis is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
Mateusz PoliwczakThat's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
Jake BaileyBut on the other hand, without this CL the `StaticValue` is used, which has the same running time, right? So maybe we don't need a oracle at all?
Jake BaileyYes, and the current version of this stack also only does its collection for func-typed nodes, which does limit things.
Oh, I see what you mean. Yeah, though it isn't much work to encapsulate it into an "oracle" struct and save it in the rare case it's used. I'm making a new bespoke visitor now just to see and it's turning out fine so far...
It was not in fact fine; the problem is that StaticValue does some stuff that is actually needed, such that getting rid of it for this purpose (no longer special casing a single-assignment to a static value) breaks things downstream. Not sure what to do here, because all of the options are kinda bad.
I can make a custom visitor/oracle that does _part_ of the work that StaticValue does, but it's quite literally 300 lines to get right even before you get to more edge cases. I could try and shove it into the ReassignOracle like I did in this PR, but that does imply actually using it for StaticValue too?
I think the idea I have is clear, "find all assignments to a function, union up the escape analysis for all of them and don't worry about nil", but acutally laying it out is sort of what I'm not sure about here and would like some guidance.
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
if Any(name.Curfn, do) {Jake BaileyThis is going to be O(n^2), because this function is O(n) and we might call it O(n) times.
Can we precompute this for a function for all variables? Similar to how ReassignOracle does it.
Mateusz PoliwczakThat's actually the next CL, where I stuck it into the oracle where I think it belongs.
compilecmp didn't show that this CL negatively affected anything, however.
Jake BaileyBut on the other hand, without this CL the `StaticValue` is used, which has the same running time, right? So maybe we don't need a oracle at all?
Jake BaileyYes, and the current version of this stack also only does its collection for func-typed nodes, which does limit things.
Jake BaileyOh, I see what you mean. Yeah, though it isn't much work to encapsulate it into an "oracle" struct and save it in the rare case it's used. I'm making a new bespoke visitor now just to see and it's turning out fine so far...
It was not in fact fine; the problem is that StaticValue does some stuff that is actually needed, such that getting rid of it for this purpose (no longer special casing a single-assignment to a static value) breaks things downstream. Not sure what to do here, because all of the options are kinda bad.
I can make a custom visitor/oracle that does _part_ of the work that StaticValue does, but it's quite literally 300 lines to get right even before you get to more edge cases. I could try and shove it into the ReassignOracle like I did in this PR, but that does imply actually using it for StaticValue too?
I think the idea I have is clear, "find all assignments to a function, union up the escape analysis for all of them and don't worry about nil", but acutally laying it out is sort of what I'm not sure about here and would like some guidance.
I think the idea I have is clear, "find all assignments to a function, union up the escape analysis for all of them and don't worry about nil", but acutally laying it out is sort of what I'm not sure about here and would like some guidance.
I think that you could just copy https://github.com/golang/go/blob/f93915339a717ad3732976c2a28cd8251e0dfdfd/src/cmd/compile/internal/devirtualize/devirtualize.go#L181-L614 and adjust it to work with values instead of types + remove the `ifaceCallExprAssigns` and `InlinedCall` method.
It basically does the same thing, but for types.
Related details
- Alan Donovan
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 1 comment![]( "Open in Gerrit")
switch n.Op() {I think we need to handle `OSWITCH`, for example:
```
foo := any(func(a *int) {escape = a})
switch foo := foo.(type) {
case func(a *int):
foo = func(a *int) {}
foo(new(int)) // might not escape
}
```I think it might miss the implicit var declaration.
No, it gets followeed via DoChildren through Any, pretty sure
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 5 comments![]( "Open in Gerrit")
v := ir.StaticValue(call.Fun)If we called `FuncSingleAssignment` here instead of `StaticValue`, shouldn't that work?
No, StaticValue does more stuff than the simple walker, and making it work basically means reimplementing StaticValue...
// non-nil RHS at the same function level (not inside a nested closure).Jake BaileyInside a loop is ok?
(i.e. this is Static Single Assignment.)
Inside a loop is fine; really, any assignment is fine since we're not determining a static value, just what the escape analyzer sees.
Acknowledged
// assignments, has any complex assignments (OAS2, OASOP, ORANGE), or
// is assigned inside a closure.Jake BaileyIs there any reason behind that, other than reducing the complexity? Just curious.
Mateusz PoliwczakLargely it was to limit scope; I don't think the existing StaticValue handled that so what I stuck in this CL didn't either.
Hmm, I think it does since it follows `OCLOSURE`?
I just wonder whether removing the `closureDepth > 0` is just enough to support this.
Turns out there's other stuff in escape analysis that stops this; it seems like crossing the closure boundary causes Addrtaken. I could safely remove it I think, but it would effectively return later in the stack.
switch n.Op() {Jake BaileyI think we need to handle `OSWITCH`, for example:
```
foo := any(func(a *int) {escape = a})
switch foo := foo.(type) {
case func(a *int):
foo = func(a *int) {}
foo(new(int)) // might not escape
}
```I think it might miss the implicit var declaration.
No, it gets followeed via DoChildren through Any, pretty sure
Acknowledged
You could do this with a `defer`, then you don't need the `abort` flag. Just `return Any(n.Func, do)`.
Done
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Bailey (Gerrit)
Jake Bailey added 3 comments![]( "Open in Gerrit")
Mateusz PoliwczakSo in general, this CL contains the "native" / minimal version of this change, but to do more, I think I'd really want to have switched over to the ReassignOracle to track these assignments. thepudds had told me that this will break things that aren't tested, though, so I'm not sure exactly how you all want me to do it.
I certainly prefer the method in my last CL in this stack where I can merge the results from all assinged funcs, rather than special casing a single func.
Jake BaileyPersonally I think we might need a specialized oracle just for this usecase. I have done that for devirtualizer and it seems to serve us well.
I think that we also have a low-hanging fruit in the escape analysis i.e. to track all possible types of interface variables, that then we can use the same trick as in the CL 771500 to improve escape analysis results. And that also seems like a specialized need (although seems like a re-use of devirtualizer logic, with tiny modifications)
But i will defer that to @k...@golang.org.
I think that's right, frankly. I'm going to kill the stack and just turn this CL into one with a new oracle, and actually handle the multi-assignment case. I think that will basically resolve all of the comments.
I tried and it didn't go incredibly; it added something like 300 lines of code. I sort of think the reassign oracle is the end-all-be-all of this...
StaticValue skips for non-PAUTO, should we do that also here? See L902 here.
Done
as.Y == nil should return true? We use that for zero-initialization.
Done
Related details
- Alan Donovan
- David Chase
- Keith Randall
- Mateusz Poliwczak
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
switch n.Op() {Jake BaileyI think we need to handle `OSWITCH`, for example:
```
foo := any(func(a *int) {escape = a})
switch foo := foo.(type) {
case func(a *int):
foo = func(a *int) {}
foo(new(int)) // might not escape
}
```I think it might miss the implicit var declaration.
Jake BaileyNo, it gets followeed via DoChildren through Any, pretty sure
Acknowledged
- Alan Donovan
- David Chase
- Jake Bailey
- Keith Randall
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mateusz Poliwczak (Gerrit)
Mateusz Poliwczak added 1 comment![]( "Open in Gerrit")
v := ir.StaticValue(call.Fun)Jake BaileyIf we called `FuncSingleAssignment` here instead of `StaticValue`, shouldn't that work?
No, StaticValue does more stuff than the simple walker, and making it work basically means reimplementing StaticValue...
Acknowledged