[PATCH] D123652: Add use condition for combine SetCCMOVMSK

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:45009
+                 NumElts <= CmpBits && CmpVal.isMask(NumElts) &&
+                 CmpOp.getNode()->hasOneUse();
   if (!IsAnyOf && !IsAllOf)
----------------
LuoYuanke wrote:
> RKSimon wrote:
> > 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.
> I think we can use IsOneUseAllOf and IsOneUseAnyOf to fix 2 the cases that we know one use should be checked first. If there are more cases show only one use is optimizable, we can fix them in another patch.
No problem,
just want to mention 2 point,:
1 this is a optimization issue not correctness issue.
2 here is no muti-uses tested before, it mean the old authors didn't consider muti-uses. 


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

https://reviews.llvm.org/D123652