- User Since
- Sep 12 2012, 3:50 AM (356 w, 5 d)
Fri, Jul 12
(excise some leftover debugging code).
Wed, Jul 3
Not adding new predicates SGTM.
Most of this looks mechanical; I don't think the MVT changes should be contentious. However, the "is65536BitVector()" predicates are starting to get a bit unwieldy to read.
LGTM, thanks for the change Oliver!
Tue, Jul 2
Mon, Jul 1
LGTM with one simplification. Thanks!
May 22 2019
Thanks for the context Hans.
May 20 2019
I think I've convinced myself that this is correct. LGTM.
May 19 2019
May 10 2019
May 9 2019
Please clang-format. There should be a space between the + operator and its operands.
May 3 2019
Looking much better. I think the TTI hook could be described better.
May 2 2019
Undo approval; was looking at an incorrect diff.
This seems reasonable to me.
May 1 2019
Apr 30 2019
LGTM, thanks for making the changes!
I don't agree; if you're going to add this canonicalization, please do it in InstCombine first.
Apr 29 2019
LGTM with a couple of nits. Thanks!
Thanks for the changes, just two comments!
Apr 26 2019
Thanks for implementing this! It's really cool.
Yes, I also have the same concerns as Roman. If we don't canonicalize to fshr in instcombine, then we shouldn't canonicalize to fshr in simplifycfg. They're both canonicalization passes.
Thanks! Much easier to review this time.
Thanks for splitting these up! They're really much easier to read.
Apr 25 2019
Please reupload with full context as described in the developer's guide.
I am not the right person to review this change. I have never made changes in this directory and don't know enough about potential impact to LGTM.
Apr 23 2019
Seems reasonable to me.
Apr 19 2019
Thanks Hal! r358743.
Apr 18 2019
Sep 18 2017
Jun 28 2017
May 25 2017
May 23 2017
+1 from me.
May 22 2017
Re-writing here because phab didn't pick up my email reply.
PR32825 seems valid and has been open for some time. Weiming, I have reverted this in r303535.
And stating that they are would be incorrect.
May 19 2017
No it won't, in the same way pre-order is not equivalent to predecessors before successors :)
Thanks for the review comments:
May 18 2017
- Patch updated
- Rebased onto NewGVN
- No known defects - LNT, spec2k, spec2k6, usual suspects run, no problems.
- All of the defects raised in SimplifyCFG's implementation have been added to the regression test suite for GVNSink.
May 15 2017
Right; that's X86's strange not-a-maxnum-not-a-maxnan behaviour where it just selects the zeroth operand.
On the other hand, some backends (e.g. X86) have only
FMIN/FMAX nodes that do not care about NaNS and the former NAN/NUM
nodes are illegal thus selection is not happening.
May 8 2017
I think this looks fine to me.
Apr 28 2017
Apr 27 2017
Ah, thanks Eli!
Apr 25 2017
Apr 21 2017
LGTM, thanks! Looks like the rest of the code was written with this in mind, but I never got around to it.
Apr 20 2017
LGTM, thanks for catching these!
Apr 18 2017
It looks good to me, but it would as I wrote this particular patch originally :) For that reason I can't effectively review it, so I'll resign.
One change requested, otherwise LGTM. Thanks for the cleanup!
Apr 11 2017
Apr 10 2017
I've spent some time coming up to speed on this in picking it up from Artyom. The million dollar question on this seems to be whether OptionalDefs are a total hack and should be ripped out entirely, or should be kept. I think in the latter case, the patch itself looks correct.
Apr 7 2017
Nice cleanup, thanks!
Apr 6 2017
It took me a while to grok this, so here's some ascii-art for the record :)
Apr 5 2017
Barring comments, my default plan is to mark the shared subset of instructions across the RUN calls that make sense.
Thanks for this fix. I'm not 100% sure that all of it is needed - comments inline. Also, I particularly like the thoroughness of the commenting in your testcases. Much appreciated! :)
Apr 4 2017
You're still checking the entire codegen output for this function. That's not what this test is about; this doesn't add any value, all it does is add maintenance burden.
Those testcases are too strict. If you just need to check that a testcase doesn't cause an assertion failure, you can avoid any FileCheck checks and put "; REQUIRES: asserts" (I think that's the syntax) to ensure it only gets run on an asserts build.
Thanks for submitting this change, but I don't understand it. The phab issue title and summary don't seem to match up, the summary isn't descriptive enough for me to grok what's going on here and you haven't provided a testcase.
Mar 31 2017
I'm confused about how this test change is actually checking for any change in behaviour. No CHECK lines have been updated - have I missed something? Is the test currently failing?
Mar 23 2017
Looks even better than Pirama's patch. Thanks!
Ouch! LGTM, thanks!
Mar 18 2017