- User Since
- Jul 7 2012, 2:54 PM (302 w, 2 d)
I think the existing testing of the fact that the parent needs invalidation makes perfect sense to switch to the more general API. Good spot. Feel free to commit.
LGTM, good spot. I'll also definitely check if this fixes the mystery stage2 miscompile we've seen on PPC. It's a good fix either way though.
Definitely need tests here.
Sat, Apr 21
Thu, Apr 19
Hang off on doing more testing. I have more failures w/ non-trivial
unswitching just in the test suite, so I already have multiple
reproductions that I'm working on.
Wed, Apr 18
OK, should be ready for review again! A much, much simpler patch now.
And fix typo.
Use the existing facilities and sink this past the point where we find actual
unswitch candidates. This should at least avoid the cost of the loop traversal
when there are no loop invariant conditions.
Tue, Apr 17
Mon, Apr 16
Arrange this test case to be slightly less brittle.
These kinds of improvements to warnings are awesome, but the way we are deploying them presents serious challenges to adoption which I think we need to address.
Sun, Apr 15
Tue, Apr 10
Mon, Apr 9
Clean up disassembler array a bit.
Fix a remaining place where we need to mark EFLAGS as used as we pass through
some of the flags.
Rebase now that the core flags change has landed.
Rebase and updates. I think this is probably ready to go in now?
Thu, Apr 5
Meh, I think this is fine as-is.
LGTM with doc fixes and Twine fixes below. Really excited about this!
Wed, Apr 4
Just wanted to explicitly say +1 to this default change (with the adjustment Richard suggested).
Tue, Apr 3
Update based on review comments.
All done. Everything just passed the machine verifier. =]
Last major issue I'm aware of with this patch addressed (see below). I think this is ready for another round of review.
Another major update.
This is looking really awesome. Starting to get into more mundane / nit picky comments below.
Mon, Apr 2
Update fixing the remaining issues and rebasing on top of the EFLAGS patch.
Major update. Addresses all but one code review comment. Also updates all tests
and adds my dedicated MIR test for this functionality.
Sun, Apr 1
Rebase after splitting out patches and after fixing some minor code review comments.
Thu, Mar 29
Given the direction this is going, I'd suggest maybe renaming the attribute. I'm thinking something like legal_vetor_width or otherwise framing this as fundamentally a legalization constraint rather than anything more fundamental. I would also really suggest doing more than just one width shift in this patch because that will help ensure that the mechanism is working in the general way as intended.
Wed, Mar 28
One thing I should clarify here:
Tue, Mar 27
Rebase and updates from a round of feedback from Paul Kocher.
Mon, Mar 26
FWIW, I'm really, really happy to have this. =D
FWIW, I'm a *huge* fan of the approach of legalizing via widening (and have advocated for this in the past). Is there any specific review feedback you're looking for here beyond what you've already got?
I have a feeling that the original bug was from a time where we generally thought this side of commandline infrastructure was going to become a more broadly used way of building commandline tools with rich commandline interfaces. However, in recent years the project has gravitated towards the infrastructure in the Option library which is used by both Clang and LLD.
Since this seems not going anywhere, removing it from my review dashboard.
Just marking that this would need rebasing on a corrected form of the underlying change. That way it doesn't show up in my queue to review.
I think this is more a question for Richard... Add me back to the reviewers if there is a specific need for my input here.
Just a question really.
I haven't really had time to look at this, but it seems somewhat unfortunate to add yet another format like this. =[ I guess I'm surprised that AMD is looking at baking this into their object file format. I'm not sure it makes sense to really consider landing this until the usage of it is at least available for review as well. While having the patches separate is good, I think it would be important to see the actual planned usage before adding all this code.
Mar 24 2018
Rebase and update. Relevant changes here:
Mar 23 2018
Mar 17 2018
FYI, Andrew Pardoe from Microsoft and I were talking and he wanted to express really strong support for getting this in and deployed. He's also offered any help that would be useful to sorting this out so there is a solid installer for VS 2017.
Mar 16 2018
Doh! Sorry these crossed. But yes, this is exactly the patch I landed and I credited you with it already! =D