[llvm] [IR] Reject a scalable byval argument in the verifier (PR #226170)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Timur Baidusenov (bai-tim)

<details>
<summary>Changes</summary>

A byval argument is copied into the caller's frame, so passing one needs its size at compile time and no target can do that for a scalable type. Rather than teach SafeStack to step around it (#<!-- -->225067), make it invalid IR: the construct breaks more than one pass, plain llc already fails on it, and nothing produces it -- AAPCS64 passes an SVE argument in Z and P registers, or indirectly through a plain pointer, never byval.

The MemCpyOpt test case that a scalable byval used to crash is dropped, as that IR is now rejected.

Fixes #<!-- -->182759.

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


4 Files Affected:

- (modified) llvm/docs/LangRef.md (+2) 
- (modified) llvm/lib/IR/Verifier.cpp (+4) 
- (modified) llvm/test/Transforms/MemCpyOpt/vscale-crashes.ll (-19) 
- (added) llvm/test/Verifier/byval-scalable.ll (+25) 


``````````diff
diff --git a/llvm/docs/LangRef.md b/llvm/docs/LangRef.md
index 4966065f2207c0..0a370719f45825 100644
--- a/llvm/docs/LangRef.md
+++ b/llvm/docs/LangRef.md
@@ -1378,6 +1378,8 @@ Currently, only the following parameter attributes are defined:
     interpreted as a call to memcpy with the allocation size of the specified type,
     instead of loading from the pointee and storing back into the copy in the type.
     In particular, the padding between field types of a struct type is still copied.
+    The type must not be a scalable type, as the size of the copy has to be known
+    at compile time.
 
     The byval attribute also supports specifying an alignment with the
     `align` attribute. It indicates the alignment of the stack slot to
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 1440b95896474f..0adc23d3bb4140 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2351,6 +2351,10 @@ void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty,
       // used on the stack.
       Check(!ByValTy->containsNonLocalTargetExtType(),
             "'byval' argument has illegal target extension type", V);
+      // The copy is placed in the caller's frame, which needs its size at
+      // compile time.
+      Check(!ByValTy->isScalableTy(),
+            "scalable 'byval' arguments are unsupported", V);
       Check(DL.getTypeAllocSize(ByValTy).getKnownMinValue() < (1ULL << 32),
             "huge 'byval' arguments are unsupported", V);
     }
diff --git a/llvm/test/Transforms/MemCpyOpt/vscale-crashes.ll b/llvm/test/Transforms/MemCpyOpt/vscale-crashes.ll
index e6ebe974cffa8a..73e314967861cd 100644
--- a/llvm/test/Transforms/MemCpyOpt/vscale-crashes.ll
+++ b/llvm/test/Transforms/MemCpyOpt/vscale-crashes.ll
@@ -1,25 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=memcpyopt -S -verify-memoryssa | FileCheck %s
 
-; Check that a call featuring a scalable-vector byval argument fed by a memcpy
-; doesn't crash the compiler. It previously assumed the byval type's size could
-; be represented as a known constant amount.
-define void @byval_caller(ptr %P) {
-; CHECK-LABEL: @byval_caller(
-; CHECK-NEXT:    [[A:%.*]] = alloca i8, align 1
-; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr align 4 [[A]], ptr align 4 [[P:%.*]], i64 8, i1 false)
-; CHECK-NEXT:    call void @byval_callee(ptr byval(<vscale x 1 x i8>) align 1 [[A]])
-; CHECK-NEXT:    ret void
-;
-  %a = alloca i8
-  call void @llvm.memcpy.p0.p0.i64(ptr align 4 %a, ptr align 4 %P, i64 8, i1 false)
-  call void @byval_callee(ptr align 1 byval(<vscale x 1 x i8>) %a)
-  ret void
-}
-
-declare void @llvm.memcpy.p0.p0.i64(ptr align 4, ptr align 4, i64, i1)
-declare void @byval_callee(ptr align 1 byval(<vscale x 1 x i8>))
-
 ; Check that two scalable-vector stores (overlapping, with a constant offset)
 ; do not crash the compiler when checked whether or not they can be merged into
 ; a single memset. There was previously an assumption that the stored values'
diff --git a/llvm/test/Verifier/byval-scalable.ll b/llvm/test/Verifier/byval-scalable.ll
new file mode 100644
index 00000000000000..7124a861c41031
--- /dev/null
+++ b/llvm/test/Verifier/byval-scalable.ll
@@ -0,0 +1,25 @@
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+; A byval argument is copied into the caller's frame, so no target can pass one
+; whose size is not known at compile time (see issue #182759).
+
+; CHECK: scalable 'byval' arguments are unsupported
+declare void @vector(ptr byval(<vscale x 4 x i32>))
+
+; CHECK: scalable 'byval' arguments are unsupported
+declare void @struct(ptr byval({ <vscale x 4 x i32> }))
+
+; CHECK: scalable 'byval' arguments are unsupported
+define void @definition(ptr byval(<vscale x 4 x i32>) %p) {
+  ret void
+}
+
+; The attribute is checked at a call site too, which is a separate path.
+
+declare void @callee(ptr)
+
+; CHECK: scalable 'byval' arguments are unsupported
+define void @call(ptr %p) {
+  call void @callee(ptr byval(<vscale x 4 x i32>) %p)
+  ret void
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/226170