[PATCH] D123652: Add use condition for combine SetCCMOVMSK

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45006
+  bool IsAnyOf = CmpOpcode == X86ISD::CMP && CmpVal.isZero() &&
+                 CmpOp.getNode()->hasOneUse();
   bool IsAllOf = (CmpOpcode == X86ISD::SUB || CmpOpcode == X86ISD::CMP) &&
----------------
I'm struggling to get a test case for the any-of pattern (the PTEST fold only occurs for all-of) - have you seen anything?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45009
+                 NumElts <= CmpBits && CmpVal.isMask(NumElts) &&
+                 CmpOp.getNode()->hasOneUse();
   if (!IsAnyOf && !IsAllOf)
----------------
xiangzhangllvm wrote:
> LuoYuanke wrote:
> > Move the check to line 45062? I'm not sure if we should check it for all cases. Currently the existing test cases don't cover all scenarios. The same for IsAnyof.
> I think all the combine should stop if the mid-node cmp result has muti-uses. And from current llvm test. We don't find any tests go bad.
> @RKSimon How do you think about it. I thinks you are more familiar with here code.
Yes, most of the folds are canonicalizations that shouldn't care much about multi-uses - it might make sense to instead split this off:

bool IsOneUseAllOf = IsAllOf && CmpOp.getNode()->hasOneUse();

And then just use IsOneUseAllOf in the PTEST fold.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123652/new/

https://reviews.llvm.org/D123652