[PATCH] D54640: [DAGCombiner] narrow truncated binops

Thu Nov 29 12:47:24 PST 2018

andreadb added a comment.

In D54640#1313266 <https://reviews.llvm.org/D54640#1313266>, @spatel wrote:

> 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.

AMD processors don’t normally rename portions of GPR registers. So, a partial write has a false dependency on the previous definition of the written register. Intel processor are able to rename portions of a GPR at the cost of a merge opcode when there is a read of full register later on.
So, it is important to care about those partial writes. However, your patch limits the change to the case where there is a single use, and one of the operand is a constant. So, in practice, your change is unlikely to ever cause problems...

> 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. :)

In this particular case it is probably not needed. That being said, partial writes are to avoid in general (in this case it doesn’t matter).

> 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