[PATCH] D37504: [x86] Fix PR34377 by disabling cmov conversion when we relied on it performing a zext of a register.

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: llvm/trunk/lib/Target/X86/X86CmovConversion.cpp:297
+                [&](MachineInstr &UseI) {
+                  return UseI.getOpcode() == X86::SUBREG_TO_REG;
+                }))
----------------
chandlerc wrote:
> aaboud wrote:
> > Thanks Chandler for the quick fix.
> > However, we can reduce the restriction to only the case where:
> > 1. We are compiling for 64bit
> > 2. The CMOV destination is 32bit.
> > 
> > All other cases has no issue with the CMOV behavior.
> > 
> > Do you agree?
> Hmm. Probably, because that's where zext is present.
> 
> However, aren't those the same conditions in which SUBREG_TO_REG will be introduced? If this is an assertion of zext-ing behavior, it can only show up due to there being zext-ing behavior of cmov itself, and that seems to be the same set of restrictions you outline.
> 
> Not sure how much more time we should spend on enhanceing the dodge of a miscompile vs. the perhaps more interesting work to handle this case elegantly and effectively.
> However, aren't those the same conditions in which SUBREG_TO_REG will be introduced? If this is an assertion of zext-ing behavior, it can only show up due to there being zext-ing behavior of cmov itself, and that seems to be the same set of restrictions you outline.

I think you are right, I was thinking about (CMOV16rr + zextTo32), but in such case the zext will not be removed, and we will not see the SUBREG_TO_REG. So, this restriction is good enough.

> Not sure how much more time we should spend on enhanceing the dodge of a miscompile vs. the perhaps more interesting work to handle this case elegantly and effectively.

Sure, I did not mean that we need to spend any more effort immediately.
The top priority is to make sure the pass has no functionality issue, which you did already.

Thinking forward, I want to be sure what more we can do.
I also prepared a patch that add the MOVrr instruction, as Dave suggested, but I need to run performance measurements before I suggest that direction.

For now, I think we are fine with this solution, at least till we see a real performance issue.


Repository:
  rL LLVM

https://reviews.llvm.org/D37504