[PATCH] D142146: [IR] Avoid creation of GEPs into vectors (in one place)

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
jsilvanus requested review of this revision.
Herald added a reviewer: mpaszkowski.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

The method DataLayout::getGEPIndexForOffset(Type *&ElemTy, APInt &Offset)
allows to generate GEP indices for a given byte-based offset.
This allows to generate "natural" GEPs using the given type structure
if the byte offset happens to match a nested element object.

With opaque pointers and a general move towards byte-based GEPs [1],
this function may be questionable in the future.

This patch avoids creation of GEPs into vectors in routines that use
DataLayout::getGEPIndexForOffset by not returning indices in that case.

The reason is that A) GEPs into vectors have been discouraged for a long
time [2], and B) that GEPs into vectors are currently broken if the element
type is overaligned [1]. This is also demonstrated by a lit test where
previously InstCombine replaced valid loads by poison. Note that
the result of InstCombine on that test is *still* invalid, because
padding bytes are assumed.
Moreover, GEPs into vectors may be outright forbidden in the future [1].

[1]: https://discourse.llvm.org/t/67497
[2]: https://llvm.org/docs/GetElementPtr.html

The test case is new. It will be precommitted if this patch is accepted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142146

Files:
  llvm/lib/IR/DataLayout.cpp
  llvm/test/Transforms/InstCombine/load-gep-overalign.ll


Index: llvm/test/Transforms/InstCombine/load-gep-overalign.ll
===================================================================
--- llvm/test/Transforms/InstCombine/load-gep-overalign.ll
+++ llvm/test/Transforms/InstCombine/load-gep-overalign.ll
@@ -16,9 +16,7 @@
 ; OVERALIGNED and NATURAL should have the same result, because the layout of vectors ignores
 ; element type alignment, and thus the representation of @foo is the same in both cases.
 ;
-; TODO: The OVERALIGNED result is incorrect.
-; First, for nonzero even indices, the valid load is replaced by poison.
-; Second, the remaining bytes at indices >= 2 are also incorrect, as apparently padding bytes
+; TODO: The OVERALIGNED result is incorrect, as apparently padding bytes
 ; are assumed as they would appear in an array. In vectors, there is no padding.
 ;
 ; NATURAL-LABEL: @test_vector_load_i8(
@@ -35,11 +33,11 @@
 ; OVERALIGNED-LABEL: @test_vector_load_i8(
 ; OVERALIGNED-NEXT:    call void @report(i64 0, i8 1)
 ; OVERALIGNED-NEXT:    call void @report(i64 1, i8 35)
-; OVERALIGNED-NEXT:    call void @report(i64 2, i8 poison)
+; OVERALIGNED-NEXT:    call void @report(i64 2, i8 0)
 ; OVERALIGNED-NEXT:    call void @report(i64 3, i8 0)
-; OVERALIGNED-NEXT:    call void @report(i64 4, i8 poison)
+; OVERALIGNED-NEXT:    call void @report(i64 4, i8 69)
 ; OVERALIGNED-NEXT:    call void @report(i64 5, i8 103)
-; OVERALIGNED-NEXT:    call void @report(i64 6, i8 poison)
+; OVERALIGNED-NEXT:    call void @report(i64 6, i8 0)
 ; OVERALIGNED-NEXT:    call void @report(i64 7, i8 0)
 ; OVERALIGNED-NEXT:    ret void
 ;
Index: llvm/lib/IR/DataLayout.cpp
===================================================================
--- llvm/lib/IR/DataLayout.cpp
+++ llvm/lib/IR/DataLayout.cpp
@@ -937,13 +937,10 @@
   }
 
   if (auto *VecTy = dyn_cast<VectorType>(ElemTy)) {
-    ElemTy = VecTy->getElementType();
-    unsigned ElemSizeInBits = getTypeSizeInBits(ElemTy).getFixedValue();
-    // GEPs over non-multiple of 8 size vector elements are invalid.
-    if (ElemSizeInBits % 8 != 0)
-      return std::nullopt;
-
-    return getElementIndex(TypeSize::Fixed(ElemSizeInBits / 8), Offset);
+    // Vector GEPs are partially broken (e.g. for overaligned element types),
+    // and may be forbidden in the future, so avoid generating GEPs into
+    // vectors. See https://discourse.llvm.org/t/67497
+    return std::nullopt;
   }
 
   if (auto *STy = dyn_cast<StructType>(ElemTy)) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142146.490601.patch
Type: text/x-patch
Size: 2451 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230119/7beadb1d/attachment.bin>