[PATCH] D54640: [DAGCombiner] narrow truncated binops
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 29 12:17:43 PST 2018
spatel added a comment.
In D54640#1313050 <https://reviews.llvm.org/D54640#1313050>, @andreadb wrote:
> LGTM.
>
> At the beginning, I was a bit concerned by the increase in the number partial register writes.
> So I started experimenting with your patch fo a bit. In practice, I couldn't come up with an obvious case where your patch introduces extra partial register stalls. Also, none of the tests modified by your patch really suffers from partial register stalls.
> The checks perfomed by your transform are quite conservative (the new combine rule is limited to pre-legalization, and it only affects binary opcodes where one of the operands is a constant). So, I am happy with it.
Thanks everyone for the reviews and comments!
I haven't thought about partial reg stalls in a while, but IIRC, AMD doesn't care in cases like these because it doesn't rename 8/16-bit regs, and Intel should be protected because we have a machine pass (-x86-fixup-bw-insts) to prevent the problem. Note that we promote ops to 32-bit (eg, D54803 <https://reviews.llvm.org/D54803>) for speed/size, so that reduces the chances for partial reg problems too. This was really just to fix that annoying rotate vs. shld/shrd bug. :)
I filed the 'movzwl' issue here:
https://bugs.llvm.org/show_bug.cgi?id=39840
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54640/new/
https://reviews.llvm.org/D54640
More information about the llvm-commits
mailing list