[PATCH] D92375: [MergeICmps] Fix missing split.

[James Y Knight via Phabricator via llvm-commits]

jyknight added a reviewer: jyknight.
jyknight added inline comments.


================
Comment at: llvm/test/Transforms/MergeICmps/X86/gep-references-bb.ll:6-8
+%Elem0 = type { i32 }
+%Elem1 = type { i32 }
+%Elem2 = type { i32 }
----------------
Indirection unnecessary; can just put i32 directly in %Triple


================
Comment at: llvm/test/Transforms/MergeICmps/X86/gep-references-bb.ll:10-12
+;; %gep does not reference an argument. Disable the optimization because
+;; otherwise the newly created gep may reference undef (after the basic block
+;; defining the pointer operand is deleted).
----------------
Comment needs to be fixed.


================
Comment at: llvm/test/Transforms/MergeICmps/X86/gep-references-bb.ll:13
+;; defining the pointer operand is deleted).
+define i1 @bug(%Triple* nonnull dereferenceable(12) %lhs, %Triple* nonnull dereferenceable(12) %rhs) {
+; CHECK-LABEL: @bug(
----------------
This example should've really been transformed into a single memcmp of 12 bytes, not a cmp of the first 4-bytes, and a memcmp of the latter 8. So, there's a missed-optimization bug in this pass, as well. (I guess it's only looking at the direct pointer arg of the GEP, not following through them the chain of GEPs to find the base object.)

That missed-opt shouldn't be fixed at the same time as the miscompile, of course, but the current test is fragile w.r.t. that missing optimization -- if we fix the pass to compile this to a single 12-byte memcmp, we'll no longer have the test case for a size==1 comparison.

I think it would be good to make the test insensitive to that opt by adding another i32 field in the struct, between the first and the second, which gets ignored by the compares -- and update the size to dereferenceable(16). That'll still trigger the misoptimization, but not be transformable into a single memcmp, so this test will still test what happens for the chain of length 1 case -- even after fixing that issue in a future change.

Maybe also include a side-affecting instruction at the top of the function which should _definitely_ not be removed, like `store i32 1, i32* @global`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92375