- User Since
- May 22 2014, 1:24 PM (213 w, 3 d)
Not sure how to make Phab select the previous version of the patch.
It's showing the patch from rL335433 now, which is not what we want of course.
Did D48508 affect any of the tests or motivation for this one?
rL335433 is the commit for this review.
Patch updated - no functional change intended from the previous rev (test diffs are identical):
- Refactored to reduce code duplication and improve readability.
- Replaced getValueSizeInBits() == 1 with getValueType() == MVT::i1.
Fri, Jun 22
I was working on a generic DAGCombiner patch for what I think are the early patterns in the motivating case:
Does this patch deal with the case if we negate the scalar before doing the splat?
In the last rev, I forgot to remove the use of the original opcode when we create the new binop.
So we could have shifts when we should have multiplies (and the tests showed that).
Oops - just noticed a typo that makes this patch wrong.
Thu, Jun 21
Committed the helper function change with rL335242, so this is only the shuffle fold now.
Adding potential reviewers based on a grep of 'deterministic' in recent commits messages. :)
No functional change from the previous rev, but use early exits to minimize indents and add TODO comments for the planned enhancements.
Wed, Jun 20
Tue, Jun 19
Added a 'TODO' comment about using a FABS-based sequence if we can't ignore -0.0.
- Improved header comment examples (use 'undef' rather than -1).
- Reimplemented logic to maximize code reuse (make use of isSingleSourceMask() internally).
- Added 'TODO' notes about allowing size-changing shuffles.
Mon, Jun 18
Do you have commit privileges?
LGTM - see inline for a nit.
I see the motivation better now, but I'm still worried about using ValueTracking.
I'm not sure how to test that potential late constant creation problem (see rL334977), but this change looks safe and does what the comment says, so LGTM.
On 2nd thought, I confused myself. :)
This patch is potentially dangerous - although it's hard to find cases where that danger propagates.
As with the earlier undef matcher changes, we have to audit the users to make sure they are not incorrectly using the matcher to propagate a value with undefs where it's not safe.
Sun, Jun 17
Uploaded the wrong draft - went overboard on the 'const &' on that last one. An ArrayRef is a const ref.
Redefine all of the Constant* functions in terms of ArrayRef<int> parent implementations. This makes the code slightly cleaner and might open up the possibility of removing some redundant code from the backend.
- Fixed undef checks to be "x == -1" rather than "x < 0".
- Added assertions for inputs and logic.
- Added more unit tests (the last group is based on the mask examples that I added to the header file code comments...would be extra embarrassing to get those wrong!).
- Reduced confusing comment for transpose (and restated the canonical TRN1 / TRN2 examples for clarity).
- Changed all 'unsigned' types to 'int' so we don't have to cast (this is always an annoyance with mask values).
- Changed variable names and added locals to try to make the logic clearer and more uniform.
Sat, Jun 16
Fri, Jun 15
LGTM too. Thanks for finding the bug and working through the steps to make good patches.
Thu, Jun 14
Adding more potential FMA reviewers. There may be some tricky bits here in the interaction between the FMF, the global settings, and the TLI hooks. See for example D28675.
Wed, Jun 13
This reads clearer to me by avoiding the potential to confuse with UB. Should the same blurb apply to nsz with -0.0?
Might want to get a 2nd opinion on 512 though since I'm not as familiar with that.