User Details
- User Since
- May 22 2014, 1:24 PM (422 w, 2 d)
Wed, Jun 22
LGTM
Mon, Jun 20
I added a similar fold here:
0399473de886
Abandoning - looks like this will need a target hook of some kind along with the more general transform in D128123.
Sun, Jun 19
LGTM
LGTM - noted a close call in x86 tests, but that can be addressed if we find real regressions.
I committed the tests with baseline results, so we have those in place independently of the code change. I then updated the tests to provide a bit more coverage. Please rebase/update after:
feb4b336acc71
Sat, Jun 18
I put up D128123 as the potential better/final patch, so we can see the current test diffs.
Fri, Jun 17
The uploaded diff is against the previous revision of this patch. Please rebase so it is against (and can be applied cleanly to) the main branch.
Thanks - let's give this a try.
It seems like it can help IR analysis, and there are potential wins for codegen too. If we find perf regressions that are not easy to reverse, then we can revert.
LGTM
LGTM - see inline for some possible minor improvements.
LGTM
Thu, Jun 16
LGTM - thanks for the thorough tests!
See inline for some minor cleanups.
I don't know enough to visualize the GEP corner-cases or the interaction with LICM, but I commented on general improvements to the patch.
Wed, Jun 15
Tue, Jun 14
Mon, Jun 13
LGTM - as noted in 21d7c3bcc646f5db73b, this pattern is probably not common because it's not the canonical IR.
LGTM
Sun, Jun 12
I'd like to see some form of load combining in IR, but we need to be very careful about how that is implemented.
Fri, Jun 10
I don't like increasing risk by combining what are really 2 independent changes into 1 patch.
Please go back to the earlier revision that only adds the new fold. We can ease the vector restrictions as a second step (I haven't seen any evidence that those cases matter, but it opens up more possibilities to miscompile in ways that are very hard to detect).
Note that we likely have FMF propagation bugs in this code based on discussion/examples in D127275. If you can put a TODO comment on this, that would be helpful. We'll probably need to add more tests with mixed FMF to demonstrate the bugs.
Note: I think we should still do the narrowing transform that I suggested in D123408, but that's independent of this fix. That narrowing commit ran into its own infinite loop due to a conflict matching constant expressions, but I can fix that.
Thu, Jun 9
If I'm seeing it correctly, this will alter D126591 or possibly make it unnecessary. I recommend implementing the symmetric TODO pattern for this patch as the next patch, and then we can see what remains.
Wed, Jun 8
LGTM
LGTM
LGTM - it may be worth looking at the related folds to see if there's some generalization that we missed (see inline comment about the other TODO comment in the test file).
https://alive2.llvm.org/ce/z/iwd4ba
This is needed for correctness, so LGTM.
It would be good to refactor foldSelectIntoOp() with a lambda as a preliminary cleanup, so we don't have to duplicate the code.
But this is an improvement even without that, so LGTM.
If this is inverting the transform from D126774, do we need 'nsz' to avoid miscompiling -0.0?
https://alive2.llvm.org/ce/z/4IMKLM
Tue, Jun 7
For reference, this was the Alive2 from the original review ( D117365 ):
https://alive2.llvm.org/ce/z/tFAcZt
Adjusted code to allow non-uniform (non-splat) vector constants on the shifted (inner) constant.
Mon, Jun 6
IIUC, the problem/fix (and it would be good to put something like this in the updated commit message for easier reference):
The previous revision/commit did not check one-use of an intermediate value that this transform re-uses. When that value has another use, an existing transform will try to invert the transform here. By adding one-use checks, we avoid the infinite loops seen with the earlier commit.