[PATCH] D140849: [Patch 1/4]: Add tests for binops with conditions/assume constraints

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

High level comment on these test is that many of them test a combination of folds, rather than their primitive parts. E.g. to test D140851 <https://reviews.llvm.org/D140851> it would be sufficient to just have an assume followed by an and+icmp to check the known bits. Going through the mul fold to test computeKnownBits() behavior is both unnecessarily indirect, and limited: We can only observe the lowest bit through that, so all tests end up doing something like `and x, 1` only. Having some end-to-end tests on motivating patterns is fine, but we mainly should be testing things in isolation. (That would probably also cut down on the total number of tests.)



================
Comment at: llvm/test/Transforms/InstCombine/icmp-binop.ll:15
+  %tobool.not = icmp eq i64 %mul, 0
+  %conv = zext i1 %tobool.not to i64
+  ret i64 %conv
----------------
All these zext + ret and be replaced by `ret i1`.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-binop.ll:295
+  tail call void @llvm.assume(i1 %or.cond7)
+  ret i64 0
+}
----------------
What's the point of these these tests that contain only assume calls?


================
Comment at: llvm/test/Transforms/InstCombine/icmp-binop.ll:615
+  %tobool.not = icmp eq i64 %v, 0
+  %conv = zext i1 %tobool.not to i64
+  ret i64 %conv
----------------
What's the point of these tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140849