[PATCH] D115755: [InstSimplify] Fold logic And to Zero

[Mehrnoosh Heidarpour via Phabricator via llvm-commits]

MehrHeidar marked 3 inline comments as done.
MehrHeidar added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2180
+    return Constant::getNullValue(Op0->getType());
+  // ((A | B) ^ B ) & ((A | B) ^ A) --> 0
+  if (match(Op0, m_c_Xor(m_c_Or(m_Value(X), m_Value(Y)), m_Deferred(Y))) &&
----------------
rampitec wrote:
> MehrHeidar wrote:
> > rampitec wrote:
> > > MehrHeidar wrote:
> > > > MehrHeidar wrote:
> > > > > rampitec wrote:
> > > > > > MehrHeidar wrote:
> > > > > > > rampitec wrote:
> > > > > > > > MehrHeidar wrote:
> > > > > > > > > rampitec wrote:
> > > > > > > > > > You could use 'm_CombineOr' for the LHS to select either deferred X or deferred Y. Then you do not need second expression.
> > > > > > > > > I replaced the comments and changed the usage of A/B with X/Y.
> > > > > > > > > Also, I tried to use the idea for usage of `m_CombineOr` to remove the second expression.
> > > > > > > > Second match shall not use m_CombineOr. Now you would incorrectly match `((X | Y) ^ X ) & ((X | Y) ^ X)`. Speaking of which it deserves a negative test.
> > > > > > > If I don't put the `m_CombineOr` on the second match, we won't be able to catch all commutes. I added a negative test now to see it won't incorrectly match your example.
> > > > > > > 
> > > > > > > I am not so confident with the current code too and prefer my first version of this patch. What do you think @rampitec ?
> > > > > > > 
> > > > > > m_CombineOr is not about commute. At the point of second match you already know your X and Y exactly.
> > > > > As I mentioned before, not using `m_CombineOr` doesn't allow me to catch all versions, so I reverted back to first version that I had.
> > > > @rampitec Would you please take a look at the patch again and check whether it's ok or not?
> > > > Thanks
> > > I still do not see why m_CombineOr does not work, can you give an example?
> > Sure, if I change the pattern match to be 
> > 
> > ```
> > match(Op0, m_c_Xor(m_c_Or(m_Value(X), m_Value(Y)), m_CombineOr(m_Deferred(X), m_Deferred(Y))) &&
> >       match(Op1, m_c_Xor(m_c_Or(m_Specific(X), m_Specific(Y)), m_Specific(X)))
> >      return Constant::getNullValue(Op0->getType());
> > 
> > ```
> > I can't  match this case ` ((X | Y) ^ X ) & ((X | Y) ^ Y)` ;
> > If I change it to 
> > ```
> > match(Op0, m_c_Xor(m_c_Or(m_Value(X), m_Value(Y)), m_CombineOr(m_Deferred(X), m_Deferred(Y))) &&
> >       match(Op1, m_c_Xor(m_c_Or(m_Specific(X), m_Specific(Y)), m_Specific(Y)))
> >      return Constant::getNullValue(Op0->getType());
> > 
> > ```
> > I can't  match this case ` ((X | Y) ^ Y ) & ((X | Y) ^ X)` ; And, if I use `m_CombineOr` on second match it's going to incorrectly match `((X | Y) ^ X ) & ((X | Y) ^ X)` this pattern.
> > 
> > So, I couldn't fully cover all cases.
> OK, I am not sure why but let it be.
Changed the code according to @spatel recommendation.


================
Comment at: llvm/test/Transforms/InstSimplify/and.ll:191
+; CHECK-LABEL: @or_xor_commute4(
+; CHECK-NEXT:    [[OR:%.*]] = or <2 x i64> [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[XOR2:%.*]] = xor <2 x i64> [[OR]], [[X]]
----------------
MehrHeidar wrote:
> rampitec wrote:
> > And in fact the test does not catch it. I think 'and' was simply dropped earlier. Maybe a duplicated 'or' will expose it or maybe not.
> Agree! I only added the previous test for the sake of your previous comment, I was not going to commit that=)
> Thank you! Changed the code so it's not a concern anymore.
These comments were related to a test that I added temporary; so they are not valid anymore. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115755