- User Since
- Oct 23 2013, 6:32 PM (361 w, 3 d)
Thu, Sep 24
Looks close to ready. Please address review comments, and we'll probably be ready for an LGTM.
Tue, Sep 22
address review comment
Mon, Sep 21
Fri, Sep 18
Thu, Sep 17
LGTM. This makes a nice starting point - as you note, we can definitely extend this.
Wed, Sep 16
Tue, Sep 15
LGTM - if you're just adding tests, you don't need separate review as long as there's nothing fancy.
Max, I think this is probably handled by the existing optimizeLoopExits if the exit counts for the condition are computable. (i.e. you're probably looking at a gap in exit count computation)
However, I think this a workaround (and a useful optimization) not a true fix. What would happen if for some reason a COPY didn't get eliminated? (Imagine no duplicates, just a single pointer being relocated, with relocated value used down the exceptional path) I don't have a test case for this, but I'd advise digging into that possibility further. I'd need to study invoke lowering more closely to understand the risk.
Mon, Sep 14
When rebasing, I discovered the tests for this were from before the operand bundle format change. Given that, I ended up dropping most of the tests. I'll land a couple more in a follow up, but overall, it seems reasonable to have most testing in the x86 target. We don't need to exercise all cornercases in both places.
LGTM as well.
Fri, Sep 11
- Your change does not include a LangRef update for the new metadata.
- The name chosen for the metadata is ambiguous. I'd also suggest phrasing it in terms of ranges, not page starts. Maybe: guaranteed_faulting_ranges, or implicit_check_faulting_ranges?
- As you note in the review, this is mixing two sets of changes in a way which makes it hard to reason about either in isolation. Please split. Either order is fine as you should be able to test either piece in isolation without the other.
Tue, Sep 8
LGTM. Good catch, and nicely structured cleanup in the process.
Fri, Sep 4
Approach generally looks reasonable. Please address nitpicks and then I plan to LGTM.
Aug 27 2020
Aug 21 2020
As a follow up, it would be nice to handle two additional optimizations:
- Canonicalize each pointer to a single index in the statepoint operand list
- Shrink the gc-live bundle to remove any pointers not needed after (1)
Aug 14 2020
Aug 13 2020
Serguei and I discussed this one fairly extensively offline. Long term, we're probably going to move to a model where we process all of the projections for a statepoint in a single pass when visiting the statepoint, but for the moment, we need to decrease the number of instcombine iterations. In particular, with recent efforts to clamp the number of iterations in debug builds, we are seeing R+A crashes in downstream tests, so this change is really a regression fix.
Jul 30 2020
LGTM. I sat down today to write a cleaner patch, and in the process convinced myself this was in fact correct. I will post a separate cleanup once this lands as I think we can have the foldMemoryOperand impl structured much more cleanly for tied operands, but doing so involves a slightly deeper rewrite of some x86 specific code than is reasonable to ask for here.
I filed a bug with a detailed description of the problem and one suggestion as to how to approach. See https://bugs.llvm.org/show_bug.cgi?id=46917, let's take discussion there (or offline).
Jul 29 2020
JFYI, I've posted (and now landed after review) the subset of this patch which is mostly NFC here: https://reviews.llvm.org/D84692
Jul 27 2020
Address Register related review comments
Jul 25 2020
I'm approving this patch not because it's perfect, but because it doesn't break anything in the existing code and iterating here is no longer productive. Once this lands, we can continue the discussion about base/derived handling with specific concrete examples.
Jul 11 2020
I took some time today to apply this patch with the attention of addressing some of the unaddressed comments and landing it to unblock progress here. However, I hit two problems.
Jul 10 2020
(Comments on code outside StatepointLowering. Minor +1 question)
Detailed comments on the new implementation. These are on the whole minor. Remember to add your new test file.
On first skim, looks much much better. I'm going to do a detailed pass through, but thank you for making the major design change requested.
Jul 9 2020
And thank you for doing this.
Jul 7 2020
I've spent several hours today wrapping my head around this patch. I think I've found a material simplification which should greatly simplify the code.
Required Change - Please introduce a runtime flag which controls how many values are handled via vregs. Default this value to zero. This will remove all existing test diffs; if it doesn't you have a bug. Then introduce a new test file, optional copied from an existing one, called statepoint-gc-regs.ll which enumerates sufficient coverage for the new feature.
Initial comments, continue to look for other suggestions.
Jul 6 2020
(drive by comments only, please don't block on me)
Jun 26 2020
Jun 25 2020
Jun 19 2020
(Just for reference, the total bit tripped me up at first, but the reasoning is that we're converting to a call, not proving the call no throw. We could still throw out.)
Denis, I'm having a really hard time wrapping my head around this code, even knowing what it is supposed to do. We need to clean this up a bit; it's not in a state where I can approve it.
Jun 15 2020
Just a thought, feel free to consider or ignore.
But you're converting an instruction which defines a vreg to one which doesn't. If the original definition vreg had uses, isn't this a miscompile?
Jun 12 2020
I can't wrap my head around why untieing operands would be correct here. It seems like we could end up with a use folded, but a def not leaving to invalid codegen? Can you either expand on the justification or discuss offline?
Jun 11 2020
LGTM with one minor stylistic question. Can be addressed in separate change if desired.
Jun 5 2020
Are two byte nops legal on all 32 bit x86? The comment seems to imply no.
Jun 4 2020
LGTM w/minor change suggested.
Agreed with Eli, simply having two passes over operands would be much cleaner.
Jun 3 2020