- User Since
- Oct 23 2013, 6:32 PM (268 w, 2 h)
Mon, Dec 10
This looks like a generally good improvement, but is it really the case that if any variable operand is a def that all must be? It would seem more general to have a list of variable operands defs and a list of variable operand uses. I have an alternate use case which would require that at some point, but I'm wondering about the current ARM backend use case at the current time.
Tue, Dec 4
LGTM w/minor comment to be addressed before landing.
Mon, Dec 3
Looks like a good step in the right direction after some cleanup. Any chance you're interested in doing the same for argument or return attributes?
Sun, Dec 2
This patch made me curious because I remembered fixing this one. Turned out that's still a patch we're carrying downstream. Oops. :(
Thu, Nov 29
LGTM in the current form. The alternate framing might also be good, but we can come back and undo this later if that works out.
Wed, Nov 28
In addition to the detailed implementation comment below, I thought of a possible semantic problem. Per the ABI, who "owns" the memory for the arguments? Does either the callee or caller assume it's immutable? If so, then your optimization needs to be restricted to a non-relocating collector since otherwise the collector might be updating a memory location assumed by AA to be immutable and bad things could happen...
Sun, Nov 25
LGTM. I'm actually a bit uncertain of the semantics of this change, but I doubt we're going to find a better reviewer, so I did my best. :)
We can continue the discussion around finalizeModule and emitStackMaps separately. It's definitely not a blocker for this patch.
Nov 12 2018
Nov 11 2018
Nov 10 2018
Nov 9 2018
A few drive by comments for the moment.
Nov 8 2018
Nov 2 2018
Drive by comments.
Oct 29 2018
One potentially problematic scenario in the current code would be if someone recursively deleted a user instruction inside an AST iteration loop. I think that would delete the pointer value, and if that was the last value in a given AS, invalidate the iteration.
Hm, can I ask what problem you were originally trying to solve? From a quick search, this code is only invoked through the value handle deleted callback. In which case, the value being deleted is the pointer itself, not the original using instruction.
p.s. This is an obvious NFC cleanup You'd be fine to land this without review.
Oct 7 2018
Starting with only high level design comments....
Oct 4 2018
LGTM w/one optional, but strongly recommended change.
Oct 3 2018
I'm not actively objecting, but I'm not sure I'd bother to pursue this intermediate state as opposed to directly addressing the root issue. If you really want to pursue this approach, I'm willing to review and signoff.
Oct 2 2018
Instead of doing the simplification recursively, wouldn't it be better to just iterate externally? That is, what if we split run into an outer function which called an inner helper routine as long as that inner routine was making progress? Or is there a concern that SimplifyCFG does not stabilize to a fixed point?
Sep 19 2018
Mostly minor comments. I considered giving a conditional LGTM, but decided it warranted one last round. I think this is very close to landing.
Sep 17 2018
Just noting a couple of known gaps (to save reviewers time):
- No verifier support (will add before submission)
- Need to add references from each support instruction type in the docs (done to avoid rebase headache)
Sep 10 2018
Another round of bugs found.
I'm approving this in the current form, despite a bit of hesitation doing so. I'd like to see the conversation around the restriction of widening based on target block to continue. I think there's a good change we'll want to tweak the semantics there, but I see that as a minor tweak, not a major redesign.
Sep 7 2018
Looked at the history in more detail, and decided I'm comfortable with this fix. So, LGTM.
Sep 6 2018
This looks correct to me, but this is so glaringly bad I'd like a second set of eyes to make sure I'm not missing something. Can someone else confirm?
Sep 5 2018
Overall, looks pretty good. One rounds of comments, but I'm expecting this to converge quickly.
LGTM w/ changes made before submit.
Aug 30 2018
Aug 29 2018
reverse last rebase, the small addition was buggy (iterator invalidation)
(slightly modified patch to reduce overhead for multiple predecessor case too)
rebase for minor issue I commented on, also ping?
Aug 24 2018
No longer needed, and extremely stale.
All component pieces have been split and landed individually.
Aug 23 2018
(snipped example for length)
Covered by existing early exit test.
- invariant store value is a phi containing invariant incoming values and the phi result depends on an invariant condition (can be handled by LICM. This patch handles?)
Unclear what you mean here.
- invariant store value is a phi containing invariant incoming values and the phi result depends on a variant condition (cannot be handled by LICM safely)
Again, I don't know what you mean by this. If the value is a phi in the loop, it's by definition not invariant.
with the right patch this time
rebase and incorporate suggestions.
Aug 22 2018
Adding requested tests, ready for review.
Aug 21 2018
Address first couple comments, another update to come before re-review justified.