[PATCH] D154126: [InstCombine] Transform (A > 0) | (A < 0) -> zext (A != 0) fold

[Hongyu Chen via Phabricator via llvm-commits]

XChy marked an inline comment as done.
XChy added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1724
+  auto MatchOrZextIcmp = [&](Value *Op0, Value *Op1) -> bool {
+    return match(Op0, m_LShr(m_Value(A), m_Constant(B))) &&
+           match(Op1, m_ZExt(m_ICmp(Pred, m_Specific(A), m_Zero())));
----------------
goldstein.w.n wrote:
> Since you only use the constant as a splat just match `B` as an `m_APInt(B)`. And make `B` a `const APInt *`.
Good advice, you remind me of `m_APInt` can match splated vector.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1737
+
+    if (Pred == ICmpInst::ICMP_SGT && MatchBAndX) {
+      Value *Cmp = Builder.CreateICmp(ICmpInst::ICMP_NE, A,
----------------
goldstein.w.n wrote:
> The `Pred == ICmpInst::ICMP_SGT` should probably be at the top when you check that logicop is `Or`.
Thanks for advice, but maybe Pred's value is decided by **MatchOrZextIcmp** function, so `Pred == xxx` must be after MatchOrZextIcmp.


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2684
+
+}
----------------
goldstein.w.n wrote:
> Can you split the tests to a seperate patch?
> 
> Basically 
> Commit 1: Tests
> Commit 2: Impl (Update tests so we can see the changes the impl causes)
> 
> Then you set the relationship between the patches using the "Edit Related Revisions" Options on Phabricator.
OK, thanks for instruction, I will get them done next morning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154126