[PATCH] D21499: Load Combine : Combine Loads formed from GEPS with negative indexes

[David Majnemer via llvm-commits]

majnemer requested changes to this revision.
majnemer added a comment.
This revision now requires changes to proceed.

I would avoid using an in-band value as a sentinel, there can be unintended consequences.  Consider what happens when the GEP index is `9223372036854775807`.

Instead, I'd use a separate bool indicating that `PrevOffset` holds a good value or make `PrevOffset` an `Optional`.


================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:141
@@ -140,3 +140,3 @@
   bool Combined = false;
-  uint64_t PrevOffset = -1ull;
-  uint64_t PrevSize = 0;
+  int64_t PrevOffset = LLONG_MAX;
+  int64_t PrevSize = 0;
----------------
While I know of no platform that LLVM supports where `LLONG_MAX != INT64_MAX`, we would still prefer `INT64_MAX`.

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:142
@@ -143,1 +141,3 @@
+  int64_t PrevOffset = LLONG_MAX;
+  int64_t PrevSize = 0;
   for (auto &L : Loads) {
----------------
Hmm, any reason why PrevSize had to become an `int64_t` ?

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/test/Transforms/LoadCombine/load-combine-negativegep.ll:1
@@ +1,2 @@
+; RUN: opt -basicaa -load-combine -instcombine -S < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
Please don't run instcombine, we try to minimize the number of passes involved in each test.


http://reviews.llvm.org/D21499