- User Since
- May 22 2014, 1:24 PM (204 w, 3 d)
Also see D24480 and the list of proposals and commits mentioned there.
Sat, Apr 21
LGTM, but please do check in the tests with baseline output first (just in case the code change is reverted, we don't want to lose the tests).
Fri, Apr 20
LGTM. You could increase diversity in the constant mask tests by not using a single-string-of-set-bits constant (eg 0x0f0f0f0f instead of 0x0000ffff).
I don't think we should be doing any of these select-of-constant transforms in instcombine.
I don't think we have what you're describing (looking through clang source
because I don't think there's any actual documentation...).
That looks ok, but I'm curious;
- How do you discover this problem?
- Is it possible to make the problem visible in a regression test (if so, please add a test)?
- The DenseMap usage is based on the code in BypassSlowDivision - does it have the same problem/fix?
Thu, Apr 19
I see that patches were applied to the Chromium sources:
I've never touched a sparc test myself, but this looks like a reasonable copy/paste. :)
Feel free to make any sparc regex changes needed to improve those CHECK lines.
Wed, Apr 18
See my comment in D45733. Sorry that I didn't get a chance to review/discuss this sooner.
If the mask is constant, right now i always unfold it.
Tue, Apr 17
The patch doesn't compile as-is. Please post a corrected patch.
Thanks for coming back to this. I think this patch is fine as one more match of a potential IR variant, but...
Since the time we started the discussion in D41353, there was a proposal to improve matching for ctpop and ctlz in D45173. I'm curious what others think about the fact that we match bswap/bitreverse in regular instcombine. Would it be better to match all of those intrinsics in the same place? Do we distinguish these based on how often we expect them to occur?
Don't know what changes are planned here, but this is on the right track. We want to have coverage of the possible canonical IR variations for various targets.
I don't understand what's going on here. Why is this patch marked 'Accepted'? In general, you shouldn't approve your own patches that you've posted for review.
Unfortunately when this if was added, no explanation was given why it is needed here. The corresponding test case is marked rdar://7726868, but I can not see what it is about.
Thanks for working on this. Adding some more potential reviewers...
Mon, Apr 16
Sun, Apr 15
Note that for the value tracking improvement, there are a couple of other passes (that I've never looked at) that use haveNoCommonBitsSet(). If it's possible to show a test improvement from this change for those passes, that would be nice.
Sat, Apr 14
LGTM - see inline for a couple of potential follow-ups.
Fri, Apr 13
Ping * 2.
Thu, Apr 12
Side note: this patch / commit should not be labeled 'NFC'.
I think the patch is correct, but it's hard to keep all of the values straight (because there are a lot!). What do you think about rearranging it a bit like this and using m_c_And():
Reopening - reverted at rL329920.
Wed, Apr 11
LGTM. I think you have a good understanding of how to write/check instcombine tests, so feel free to add those as needed without pre-commit review unless you're not sure about something.
Tue, Apr 10
Fixed the call to dominates() to use the phi's block, not the phi itself.
The tests here demonstrate the overlap between instcombine and reassociate, and based on feedback I've gotten, I'd say that rL170471 was a mistake for instcombine. It created a mini-reassociation pass within instcombine.
LGTM - see inline for a nit.
- Move the dominance check, so it only happens once. This matches the logic in InstSimplify that handles the simpler case of a phi where all incoming values are identical except for undefs.
- Added a debug statistic to count how often this fires.
Mon, Apr 9
LGTM (assuming the backend gets this right in all cases and we have tests for that).