[PATCH] D69627: [SLP]Fix PR43799: Crash on different sizes of GEP indices.

Wed Oct 30 13:25:25 PDT 2019

lebedev.ri added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4098-4099
+          auto *CI = cast<ConstantInt>(V);
+          V = ConstantExpr::getIntegerCast(CI, Ty,
+                                           CI->getValue().isSignBitSet());
+        }
----------------
xbolva00 wrote:
> ABataev wrote:
> > spatel wrote:
> > > ABataev wrote:
> > > > spatel wrote:
> > > > > I think it's already a fuzzer corner case that we have different types in the bug report example, but let's take that 1 step further: what if the index type in IR is larger than getIntPtrType()? We might illegally truncate a large constant value. 
> > > > > 
> > > > > Safer to just give up completely if we find mismatched GEP index types?
> > > > Maybe, just give up if GEP at least one index type is larger than intptr?
> > > That should work, but it seems like we are adding code complexity for no real-world reason. Unless I'm misunderstanding - do we have real code / tests where the GEP indexes are different sizes?
> > It does not add a lot of complexity here, just a few additional lines of code.
> @spatel:
> Recent memcmp related patch uses CreateGEP_64 (dont know exact name atm; index is i64) - could it be affected with your proposed stronger bail out?. We should be careful to not pessimize the code generated by own passes.
https://llvm.org/docs/LangRef.html#langref-datalayout
> Data Layout
> p[n]:<size>:<abi>:<pref>:<idx>
> This specifies the size of a pointer and its <abi> and <pref>erred alignments for address space n. **The fourth parameter <idx> is a size of index that used for address calculation.** If not specified, the default index size is equal to the pointer size. All sizes are in bits. The address space, n, is optional, and if not specified, denotes the default address space 0. The value of n must be in the range [1,2^23).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69627