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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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());
+        }
----------------
lebedev.ri wrote:
> 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).
For reference, I think the patch that @xbolva00 mentioned is:
D69507

(...unfortunately, expandmemcmp is still a codegen pass - D60318 / rL371507 - so that particular case can't be in play yet for the question/concern raised in this patch)

Given the LangRef specification, I'm surprised that we're encouraging hard-coding the index size via the Builder API rather than deriving it from the DataLayout.

So it seems like we've made a mess, and now we want to deal with that mess here. I still don't see any real-world reason for this patch to try so hard, but if we insist on it, then we should be using:
  DL->getIndexTypeSizeInBits() 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69627