User Details
- User Since
- May 22 2014, 1:24 PM (347 w, 2 d)
Today
Yesterday
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).
Wed, Dec 30
See inline comments to avoid bot failures - otherwise, LGTM. Thanks for the cleanup!
Tue, Dec 29
We're trying to (very slowly...) move away from needing the global/target options. Also, I'm not sure how those translate with GlobalISel.
If there's a real-world case where the target options do not match the fast-math-flags on the IR, we should investigate if that was unexpected. Otherwise, I would prefer that we not add to the uses of target options.
Mon, Dec 28
Sat, Dec 26
Tue, Dec 22
LGTM - there's also a code pattern like this in DAGCombiner::rebuildSetCC(), so it's hopefully safe. But as noted, we can probably do better by changing the structure a bit.
Mon, Dec 21
LGTM
Please review the comments in D16337 - especially:
https://reviews.llvm.org/D16337#331117
LGTM - the only diff from the earlier rev is that we now check the dominator tree to confirm that the shift ops are safe to hoist.
Sun, Dec 20
Thank you for working on this! Looking back at the bug comments (and adding reviewers based on those comments), this is a step towards killing undef that has been discussed for a long time now. :)
Fri, Dec 18
Dec 17 2020
Patch updated:
Adjusted order/spelling of offset checks as suggested. Also added a test (gep012) just below the offset cut-off, so we have positive and negative tests at the boundary condition.
Patch updated:
- Added constraints on address offset with respect to vector element size.
- Added negative tests for those.
- Added asserts to make sure address assumptions are valid.
- Added code comment about offset math (negation) logic.
Code changes look fairly mechanical at this point, so if the output is as expected, LGTM
Dec 16 2020
Patch updated:
D93397 / D93406 improved the basic alignment logic, so that is adjusted here to include the effect of gep offset.
In the last test diff, note that the final alignment is not the same as either of the starting alignments.