Skip to content

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.
@lukel97 lukel97 requested review from arsenm, fhahn and nikic February 22, 2024 16:40
@llvmbot llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Feb 22, 2024
@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.

@lukel97 lukel97 merged commit b0edc1c into llvm:main Feb 22, 2024
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

Labels

llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms

3 participants