[PATCH] D101720: [InstCombine] generalize select + select/and/or folding using implied conditions

Mon May 3 07:51:09 PDT 2021

aqjune added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2596
+  assert(SI.getType() == CondVal->getType() &&
+         "foldAndOrOfSelectUsingImpliedCond deals with i1 typed operands only");
+
----------------
nikic wrote:
> aqjune wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > I'm not sure about this assert. Can't you end up with an  i1 CondVal, but an <n x i1> SI here?
> > > To clarify, I do think you need to check for that case, but as a normal condition, not an assert. You do check this before calling the helper for the and/or case, but I don't think anything ensures it for the case where the root instruction is a select.
> > > 
> > > Could you please add a test for this kind of scalar/vector mismatch?
> > I think removing runtime check is fine: isImpliedCondition is handling this case well (simply returning None currently). Also, the created SelectInst does not contain Cond as well.
> > If isImpliesCondition is enhanced to deal with vector+nonvector ops, this code gets benefit as well.
> > 
> > I added the scalar/vector code mismatch test as well (a_true_implies_b_true_vec).
> Okay, if isImpliedCondition already checks this, then it's fine. In that case, should we also drop the type check when this helper is called for and/or?
It needed i1 or vec i1 check, but it could be relaxed as suggested, thanks. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101720