[PATCH] D148321: [ConstraintElimination] Add tests to check for transfering facts from sgt to ugt.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for adding the extra tests. A few suggestions about additional coverage inline



================
Comment at: llvm/test/Transforms/ConstraintElimination/transfer-signed-facts-to-unsigned.ll:599
 
+define i1 @sgt_to_ugt(i8 %a) {
+; CHECK-LABEL: @sgt_to_ugt(
----------------
Could you add a variant where we also check `icmp ugt i8 %a, 1` (negative test)


================
Comment at: llvm/test/Transforms/ConstraintElimination/transfer-signed-facts-to-unsigned.ll:626
+  %cmp = icmp sgt i8 %a, %b
+  %cmp.2 = icmp sgt i8 %b, 0
+  call void @llvm.assume(i1 %cmp)
----------------
please also add a variant of the test where we have no info about `%b`'s range (i.e. no check for %b).


================
Comment at: llvm/test/Transforms/ConstraintElimination/transfer-signed-facts-to-unsigned.ll:642
+entry:
+  %cmp = icmp sgt i8 %a, -1
+  call void @llvm.assume(i1 %cmp)
----------------
please also add a test with a different negative value.


================
Comment at: llvm/test/Transforms/ConstraintElimination/transfer-signed-facts-to-unsigned.ll:645
+  %t.1 = icmp ugt i8 %a, -1
+  ret i1 %t.1
+}
----------------
please don't use `%t.x` prefix for values that are not expected to be simplified; better just use `%c.` (here and below)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148321