- User Since
- Oct 23 2013, 6:32 PM (256 w, 5 d)
Wed, Sep 19
Mostly minor comments. I considered giving a conditional LGTM, but decided it warranted one last round. I think this is very close to landing.
Mon, Sep 17
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)
Mon, Sep 10
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.
Fri, Sep 7
Looked at the history in more detail, and decided I'm comfortable with this fix. So, LGTM.
Thu, Sep 6
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?
Wed, Sep 5
Overall, looks pretty good. One rounds of comments, but I'm expecting this to converge quickly.
LGTM w/ changes made before submit.
Thu, Aug 30
Wed, Aug 29
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.
Strip out the call hoisting bits until a future patch.
Rebase on top of tests, with test updates.
Date: Tue Aug 21 10:59:11 2018
New Revision: 340312
Aug 20 2018
Max, I landed the changes for the test separately. I applied your check-label request, but not one about single iteration loops since the test file is filed with such. Anyone who wants to specialize single iteration loops has some work to do later anyways, so I didn't see any value.
Aug 17 2018
Hold this one please. I have some concerns about the naming, but have lost track of the dependent patches and can't fully review. Once dependent changes have landed, let's revisit.
LGTM. Don't see where you're going with this, but the patch is obvious correct and doesn't make anything particularly messy.
Aug 16 2018
I still feel the framing comment could use some improvement, but I don't have anything specific I can suggest.
Detailed comments inline.
Thinking about it, I think it may be worth introducing a separate isGuaranteedToExecuteBeforeLoopExit with the old logic. While LICM's hoisting logic needs the "must execute if loop entered" property, load store promotion actually only needs "store executes before any taken exits" property. (We do need one "must execute" dereferencing instruction, but it doesn't have to be the store.)
Aug 15 2018
Ok, I think we're talking past each other a bit. I see these both as forms of predication. It sounds like you have a slightly different view; I'll try to ask clarifying questions in the right spots. I think we have different mental models here and I'm trying to understand where that difference is.
I'm missing why this is relevant? It happens to involve a volatile access as an AliasSet, but seems otherwise unrelated. From what I can tell, the "volatile implies mod" thing hasn't been true for years.
Max convinced me that there's no algorithmic complexity change implied here. Given the getLoopExits code walks all edges leaving blocks in the loop, both the old and new code is O(outbound edges of blocks in loop).
Aug 14 2018
Not that I know of. We (Azul) are using the statepoint infrastructure which shares many of the same ideas, but we are not using either STACKMAP or PATCHPOINT. I suspect we could generalize if needed, patches and discussion welcome!