[PATCH] D55538: [InterleavedAccessAnalysis] Fix integer overflow in insertMember.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:261
     Factor = std::abs(Stride);
     assert(Factor > 1 && "Invalid interleave factor");
 
----------------
efriedma wrote:
> Not part of your patch, I guess, but do we have any particular reason to expect that Stride does not equal INT_MIN?
I don't think so, I'll check and address it separately.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:284
+    if (KeyLong > std::numeric_limits<int32_t>::max() - 2 ||
+        KeyLong < std::numeric_limits<int32_t>::min() + 2)
+      return false;
----------------
fhahn wrote:
> efriedma wrote:
> > Not sure I understand the "-2" and "+2" here; is the issue that it collides with the default DenseMap tombstone otherwise?  Can we use some map that doesn't randomly reserve certain integers?  Otherwise, please add an explicit note, and check for the tombstone/empty keys separately from the overflow check.
> > 
> > Do we have some overflow-checked addition helper we could use instead of writing out the overflow check here?
> Yep it's a tombstone check. I guess we could use std::unordered_map, will that have a perf impact?
> 
> I'll check again if there are some overflow helpers anywhere, but if not then it might be worth adding some.
I've split out the tombstone handling to D59050 and updated the code to use CheckedArithmetic.h's helper functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55538