- User Since
- Oct 23 2013, 6:32 PM (354 w, 2 d)
Thu, Jul 30
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).
Wed, Jul 29
JFYI, I've posted (and now landed after review) the subset of this patch which is mostly NFC here: https://reviews.llvm.org/D84692
Mon, Jul 27
Address Register related review comments
Sat, Jul 25
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.
Sat, Jul 11
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.
Fri, Jul 10
(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.
Thu, Jul 9
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
Jun 1 2020
LGTM w/required changes applied before commit. If you disagree on any point, further review needed and LGTM does not apply.
Let's discuss offline.
May 31 2020
May 30 2020
May 29 2020
Please pull out a purely NFC patch without any of the first-N-vregs bits which is purely functional. (i.e. replaces the existing target node with the generic node, introduces the node type and wrapper class, but does nothing else.)
The alternative here is not "do we relocate undef". It's do we let the backend chose arbitrary values for an undef operand. Said more strongly, the compiler is allowed to pick any value it wants for undef, and setting it to an arbitrary constant seems fine from a legality perspective.
Mostly minor comments on style to help drive review forward. This is a drive by comment, don't wait on me please.
May 28 2020
Closed via commit 587fa99cfdb7d2a97143ba20ed8e8face57aa01c
At the moment, no. Or more accurately, I haven't thought about it enough yet to be comfortable revising the interface. We can always revise later if needed.
Closed w/commit 1af3705c7fe23db9d5308bfdf07bfbd04398b895
May 27 2020
Make a deeper change to the interface to simplify the semantic description of the patch and avoid introducing something we'd have to fix later anyway.
I can't quite parse what you meant with that last sentence.
May 26 2020
Fix a couple of stylistic points noticed when looking at the web diff
May 22 2020
Just to be clear, if the indices are the same, gc.relocates would already be removed right? i.e. the thing you're trying to solve is that the standpoint can have two operands which are the same value and thus two gc.relocates which are equivalent but not textually identical? Just want to make sure I understand the problem properly.
May 6 2020
May 4 2020
This patch bugged me when I first saw it go by; finally got back to looking at it today.
Apr 17 2020
Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.