Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Loads] Fix crash in isSafeToLoadUnconditionally with scalable accessed type #82650

Merged

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Feb 22, 2024

This fixes #82606 by updating isSafeToLoadUnconditionally to handle fixed sized loads from a scalable accessed type.

…ed type

This fixes llvm#82606 by updating isSafeToLoadUnconditionally to handle fixed sized
loads from a scalable accessed type.
@llvmbot
Copy link
Member

llvmbot commented Feb 22, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Luke Lau (lukel97)

Changes

This fixes #82606 by updating isSafeToLoadUnconditionally to handle fixed sized loads from a scalable accessed type.


Full diff: https://github.com/llvm/llvm-project/pull/82650.diff

2 Files Affected:

  • (modified) llvm/lib/Analysis/Loads.cpp (+3-3)
  • (added) llvm/test/Transforms/VectorCombine/RISCV/load-widening.ll (+22)
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 6bf0d2f56eb4eb..5916d2ab48ecec 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -364,7 +364,7 @@ bool llvm::isSafeToLoadUnconditionally(Value *V, Align Alignment, APInt &Size,
 
   if (Size.getBitWidth() > 64)
     return false;
-  const uint64_t LoadSize = Size.getZExtValue();
+  const TypeSize LoadSize = TypeSize::getFixed(Size.getZExtValue());
 
   // Otherwise, be a little bit aggressive by scanning the local block where we
   // want to check to see if the pointer is already being loaded or stored
@@ -414,11 +414,11 @@ bool llvm::isSafeToLoadUnconditionally(Value *V, Align Alignment, APInt &Size,
 
     // Handle trivial cases.
     if (AccessedPtr == V &&
-        LoadSize <= DL.getTypeStoreSize(AccessedTy))
+        TypeSize::isKnownLE(LoadSize, DL.getTypeStoreSize(AccessedTy)))
       return true;
 
     if (AreEquivalentAddressValues(AccessedPtr->stripPointerCasts(), V) &&
-        LoadSize <= DL.getTypeStoreSize(AccessedTy))
+        TypeSize::isKnownLE(LoadSize, DL.getTypeStoreSize(AccessedTy)))
       return true;
   }
   return false;
diff --git a/llvm/test/Transforms/VectorCombine/RISCV/load-widening.ll b/llvm/test/Transforms/VectorCombine/RISCV/load-widening.ll
new file mode 100644
index 00000000000000..49d0d4216c52b2
--- /dev/null
+++ b/llvm/test/Transforms/VectorCombine/RISCV/load-widening.ll
@@ -0,0 +1,22 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -passes=vector-combine -S -mtriple=riscv32 -mattr=+v | FileCheck %s -check-prefixes=CHECK,RV32
+; RUN: opt < %s -passes=vector-combine -S -mtriple=riscv64 -mattr=+v | FileCheck %s -check-prefixes=CHECK,RV64
+
+define void @fixed_load_scalable_src() {
+; CHECK-LABEL: define void @fixed_load_scalable_src(
+; CHECK-SAME: ) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    store <vscale x 4 x i16> zeroinitializer, ptr null, align 8
+; CHECK-NEXT:    [[TMP0:%.*]] = load <4 x i16>, ptr null, align 8
+; CHECK-NEXT:    [[TMP1:%.*]] = shufflevector <4 x i16> [[TMP0]], <4 x i16> zeroinitializer, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 poison, i32 poison, i32 poison, i32 poison>
+; CHECK-NEXT:    ret void
+;
+entry:
+  store <vscale x 4 x i16> zeroinitializer, ptr null
+  %0 = load <4 x i16>, ptr null
+  %1 = shufflevector <4 x i16> %0, <4 x i16> zeroinitializer, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 poison, i32 poison, i32 poison, i32 poison>
+  ret void
+}
+;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+; RV32: {{.*}}
+; RV64: {{.*}}

@lukel97 lukel97 requested a review from dtcxzyw February 22, 2024 16:40
; RUN: opt < %s -passes=vector-combine -S -mtriple=riscv32 -mattr=+v | FileCheck %s -check-prefixes=CHECK,RV32
; RUN: opt < %s -passes=vector-combine -S -mtriple=riscv64 -mattr=+v | FileCheck %s -check-prefixes=CHECK,RV64

define void @fixed_load_scalable_src() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VectorCombine::widenSubvectorLoad triggers the crash here when calling isSafeToLoadUnconditionally. Let me know if there's a better place to test for this!

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

}
;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
; RV32: {{.*}}
; RV64: {{.*}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the RV32/RV64 prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have them initially but UTC complained about differing CHECK lines?

WARNING: Found conflicting asm under the same prefix: 'CHECK'!

Inspecting the actual check lines it seems to be the same though bar the attributes, which is why I presume it's complaining. Dropped them anyway.

llvm/test/Transforms/VectorCombine/RISCV/load-widening.ll Outdated Show resolved Hide resolved
@lukel97 lukel97 merged commit b0edc1c into llvm:main Feb 22, 2024
3 of 4 checks passed
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 22, 2024
…ed type (llvm#82650)

This fixes llvm#82606 by updating isSafeToLoadUnconditionally to handle
fixed sized loads from a scalable accessed type.

(cherry picked from commit b0edc1c)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 23, 2024
…ed type (llvm#82650)

This fixes llvm#82606 by updating isSafeToLoadUnconditionally to handle
fixed sized loads from a scalable accessed type.

(cherry picked from commit b0edc1c)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RISC-V] Clang crashes when building with optimization level 2
3 participants