This patch introduces recognition of table-based ctz implementation during the AggressiveInstCombine.
This fixes the [0].
[0] https://bugs.llvm.org/show_bug.cgi?id=46434
TODO: Get the data on SPEC benchmark.
Differential D113291
[AggressiveInstCombine] Lower Table Based CTTZ djtodoro on Nov 5 2021, 8:58 AM. Authored by
Details This patch introduces recognition of table-based ctz implementation during the AggressiveInstCombine. [0] https://bugs.llvm.org/show_bug.cgi?id=46434 TODO: Get the data on SPEC benchmark.
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions @djtodoro Do you have any time to update this? Otherwise do you mind we take it over and we can update it and get it reviewed. Thanks. Comment Actions
Comment Actions Why are some test files still specifying a triple in the RUN line? It would be good to consolidate tests into less files if possible with better names/comments to explain exactly what differences are being tested in the sequence of tests. There should also be negative tests (wrong table constants, wrong magic multiplier, wrong shift amount, etc), so we know that the transform is not firing on mismatches. Comment Actions Leftovers. Thanks.
I will remove one redundant test. There are C producers as well as some top-level comments that explain what it should test (if the comment is needed). Furthermore, thanks for the suggestion for adding some negative tests - will add it.
Comment Actions Thanks for the comments.
Comment Actions
Comment Actions Thanks for the updates. I don't think I have anything else than is what is below. Any other comments from anyone else?
Comment Actions LGTM
Comment Actions Thanks for the updates. LGTM
Comment Actions Hi, I'm seeing a heap-use-after-free in AggressiveInstCombine in a build shortly after this landed, within a function added here. test/Transforms/AggressiveInstCombine/X86/sqrt.ll fails as follows under asan: https://gist.github.com/zygoloid/a270e65d32ab5b05504b3b0d5717f83b Please can you fix or revert? Comment Actions I've gone ahead and reverted for now to unblock things. Let me know if you're not able to reproduce the ASan failure and I can dig more into it. Comment Actions What was the bug, how was it fixed, and is there a new test to verify the fix? That should have been mentioned in the new commit message. Comment Actions You are right. I reverted the recommit, and I will recommit it with proper message, sorry I missed it. :/ Actually, the issue was that your patch D129167 introduced eraseFromParent and the tryToRecognizeTableBasedCttz would try to use the instruction (dyn_cast) after free. I just moved the tryToRecognizeTableBasedCttz above foldSqrt. I guess it does not need any additional test case. Comment Actions Ah, I see. Please put a comment on that call then - foldSqrt (or any other erasing transform) needs to be accounted for (last in the loop for now), or we might hit that bug. And yes, looks like we don't need another test since the existing regression test was flagged by the asan bot. |