User Details
- User Since
- May 22 2014, 1:24 PM (348 w, 5 d)
Today
LGTM
Thanks - LGTM.
Use something other than fast on the 2nd call to provide better test coverage.
Please update the CHECK lines with utils/update_test_checks.py (use "--function=powi" if you want to avoid diffs on other functions).
I haven't looked at the trade-offs, but if we can do this transform later ( SelectionDAGLegalize::ConvertNodeToLibcall() ? ) that would seem to be more flexible...and smaller patch?
For reference:
D8131 - added support for external vector library calls
D70107 - added inject-tli-mappings
Yesterday
Sun, Jan 24
Sat, Jan 23
Fri, Jan 22
Thu, Jan 21
Wed, Jan 20
The cost model part of the patch LGTM. I think that someone more familiar with the loop vectorizer should have a look at that part.
Tue, Jan 19
I haven't looked at x86 gather/scatter code at all - ping @pengfei for more thoughts on that.
I'm not up-to-date with the current state of MVE and other Arm ISA additions, so others should feel free to chime in. :)
Instead of adding a specialization parameter for MLA to normal math reductions, would it be simpler/cleaner to add a dedicated cost model API for MLA? For example, we already distinguish min/max from other ops via getMinMaxReductionCost().
I have not followed the plans for changes to the IR, etc. But this doesn't seem to conflict with any optimizations that SLP tries to do, so LGTM.
Mon, Jan 18
Patch updated:
- Change cost calc to use getMinMaxReductionCost() (this is copied from the existing integer min/max code).
- Added test coverage/comments to demonstrate FMF constraints.
Patch updated: I changed some variable names with d1c4e85 and missed rebasing this diff.
Sat, Jan 16
Fri, Jan 15
Thu, Jan 14
Wed, Jan 13
Tue, Jan 12
Mon, Jan 11
Please upload patch with context:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
Adding more potential reviewers based on D88669 (where this code was added).
Anyone see problems with this Alive2 implementation using count-leading-*?
https://alive2.llvm.org/ce/z/SWxadd
Ping.
Note: I added a bitwidth > 2 constraint to be safe and avoid degenerate narrow bit patterns, but I'm not sure how to write negative tests for that would actually survive to this point in instcombine (instsimplify appears to reduce those first).
Sun, Jan 10
Fri, Jan 8
Thu, Jan 7
Wed, Jan 6
LGTM - see inline for possible missed comment.
I haven't followed the details enough to comment on the changes directly, but thanks for the cleanup! The mismatched signed/unsigned cost model APIs are/were a mess.
Tue, Jan 5
LGTM - other reviewers are probably more familiar with InterleavedAccess than me, so might want to wait for a 2nd opinion.
I made another assert suggestion for that code. Similar to the assert that you have proposed in this patch, we could add the asserts before this patch just to make the shuffle assumptions clearer (assuming those are correct assertions).
Does this work with scalars too? Let me know if I missed the test(s).
Mon, Jan 4
Patch updated:
Fixed semi-colon typos in test comments.
Fri, Jan 1
Thu, Dec 31
LGTM - please pre-commit the test with the current (trunk) codegen, so we see the diff (and don't lose the test in case we have to revert).