[PATCH] D60600: [InstCombine] Fix a vector-of-pointers instcombine undef bug.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

We clearly need to fix the bug (crash). @reames or someone else with more GEP experience should have a look though. I've just pointed out some minor bits to clean up.



================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1178
+
+    auto simplifyGEPOperand = [&](unsigned i, bool isIndexStruct) -> bool {
       if (isa<UndefValue>(I->getOperand(i))) {
----------------
nit: variables/parameters should (unfortunately) be capitalized, so:
i -> Index
isIndexStruct -> IsIndexStruct
?

Also, don't need to explicitly state the return type?


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1187
+        // If we have a vector of indices into a struct element of the GEP, and
+        // change a single element this into an undef while preserving the
+        // others, that breaks the guarantee that each index of a
----------------
element this -> element of this?


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1200-1202
+    if (simplifyGEPOperand(0, false)) {
+      return nullptr;
+    }
----------------
No need for braces here.


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1205-1209
+    for (unsigned i = 1; i < I->getNumOperands(); i++, GTI++) {
+      if (simplifyGEPOperand(i, GTI.isStruct())) {
+        return nullptr;
       }
     }
----------------
No need for braces here.


================
Comment at: test/Transforms/InstCombine/vector-of-pointers.ll:1-4
+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+%foo = type { float, i8 }
+
----------------
I would prefer to put this test in the same file that D57468 used since that was where the bug was introduced:
llvm/trunk/test/Transforms/InstCombine/vec_demanded_elts.ll

Also, please use this script to automatically create/update the CHECK lines (that makes it easier to update in the future):
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60600