[IR] Unify max alignment for arguments with generic max align. #99257

efriedma-quic · 2024-07-16T23:24:47Z

The 2^14 limit was completely arbitrary; the generic limit is still arbitrary, but at least it's the same arbitrary limit as everything else.

While I'm here, also add a verifier check for the ByValOrByRefSize.

The 2^14 limit was completely arbitrary; the generic limit is still arbitrary, but at least it's the same arbitrary limit as everything else. While I'm here, also add a verifier check for the ByValOrByRefSize.

llvmbot · 2024-07-16T23:25:17Z

@llvm/pr-subscribers-llvm-ir

Author: Eli Friedman (efriedma-quic)

Changes

The 2^14 limit was completely arbitrary; the generic limit is still arbitrary, but at least it's the same arbitrary limit as everything else.

While I'm here, also add a verifier check for the ByValOrByRefSize.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetCallingConv.h (+4-4)
(modified) llvm/lib/IR/Verifier.cpp (+23-15)
(modified) llvm/test/Verifier/param-align.ll (+7-7)
(modified) llvm/test/Verifier/param-attr-align.ll (+2-2)
(modified) llvm/test/Verifier/param-ret-align.ll (+6-6)

diff --git a/llvm/include/llvm/CodeGen/TargetCallingConv.h b/llvm/include/llvm/CodeGen/TargetCallingConv.h
index 0ff4e1fe657f4..cb0055633f4f3 100644
--- a/llvm/include/llvm/CodeGen/TargetCallingConv.h
+++ b/llvm/include/llvm/CodeGen/TargetCallingConv.h
@@ -45,9 +45,9 @@ namespace ISD {
     unsigned IsHva : 1;        ///< HVA field for
     unsigned IsHvaStart : 1;   ///< HVA structure start
     unsigned IsSecArgPass : 1; ///< Second argument
-    unsigned MemAlign : 4;     ///< Log 2 of alignment when arg is passed in memory
-                               ///< (including byval/byref). The max alignment is
-                               ///< verified in IR verification.
+    unsigned MemAlign : 6; ///< Log 2 of alignment when arg is passed in memory
+                           ///< (including byval/byref). The max alignment is
+                           ///< verified in IR verification.
     unsigned OrigAlign : 5;    ///< Log 2 of original alignment
     unsigned IsInConsecutiveRegsLast : 1;
     unsigned IsInConsecutiveRegs : 1;
@@ -67,7 +67,7 @@ namespace ISD {
           IsSecArgPass(0), MemAlign(0), OrigAlign(0),
           IsInConsecutiveRegsLast(0), IsInConsecutiveRegs(0),
           IsCopyElisionCandidate(0), IsPointer(0) {
-      static_assert(sizeof(*this) == 3 * sizeof(unsigned), "flags are too big");
+      static_assert(sizeof(*this) == 4 * sizeof(unsigned), "flags are too big");
     }
 
     bool isZExt() const { return IsZExt; }
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 75a53c1c99734..c5c407637cbf3 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -324,13 +324,6 @@ namespace {
 
 class Verifier : public InstVisitor<Verifier>, VerifierSupport {
   friend class InstVisitor<Verifier>;
-
-  // ISD::ArgFlagsTy::MemAlign only have 4 bits for alignment, so
-  // the alignment size should not exceed 2^15. Since encode(Align)
-  // would plus the shift value by 1, the alignment size should
-  // not exceed 2^14, otherwise it can NOT be properly lowered
-  // in backend.
-  static constexpr unsigned ParamMaxAlignment = 1 << 14;
   DominatorTree DT;
 
   /// When verifying a basic block, keep track of all of the
@@ -2021,31 +2014,43 @@ void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty,
   }
 
   if (isa<PointerType>(Ty)) {
+    if (Attrs.hasAttribute(Attribute::Alignment)) {
+      Align AttrAlign = Attrs.getAlignment().valueOrOne();
+      Check(AttrAlign.value() <= Value::MaximumAlignment,
+            "huge alignment values are unsupported", V);
+    }
     if (Attrs.hasAttribute(Attribute::ByVal)) {
-      if (Attrs.hasAttribute(Attribute::Alignment)) {
-        Align AttrAlign = Attrs.getAlignment().valueOrOne();
-        Align MaxAlign(ParamMaxAlignment);
-        Check(AttrAlign <= MaxAlign,
-              "Attribute 'align' exceed the max size 2^14", V);
-      }
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getByValType()->isSized(&Visited),
             "Attribute 'byval' does not support unsized types!", V);
+      Check(DL.getTypeAllocSize(Attrs.getByValType()).getKnownMinValue() <
+                (1ULL << 32),
+            "huge 'byval' arguments are unsupported", V);
     }
     if (Attrs.hasAttribute(Attribute::ByRef)) {
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getByRefType()->isSized(&Visited),
             "Attribute 'byref' does not support unsized types!", V);
+      Check(DL.getTypeAllocSize(Attrs.getByRefType()).getKnownMinValue() <
+                (1ULL << 32),
+            "huge 'byref' arguments are unsupported", V);
     }
     if (Attrs.hasAttribute(Attribute::InAlloca)) {
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getInAllocaType()->isSized(&Visited),
             "Attribute 'inalloca' does not support unsized types!", V);
+      Check(DL.getTypeAllocSize(Attrs.getInAllocaType()).getKnownMinValue() <
+                (1ULL << 32),
+            "huge 'inalloca' arguments are unsupported", V);
     }
     if (Attrs.hasAttribute(Attribute::Preallocated)) {
       SmallPtrSet<Type *, 4> Visited;
       Check(Attrs.getPreallocatedType()->isSized(&Visited),
             "Attribute 'preallocated' does not support unsized types!", V);
+      Check(
+          DL.getTypeAllocSize(Attrs.getPreallocatedType()).getKnownMinValue() <
+              (1ULL << 32),
+          "huge 'preallocated' arguments are unsupported", V);
     }
   }
 
@@ -3511,12 +3516,15 @@ void Verifier::visitCallBase(CallBase &Call) {
         "not allowed. Please use the @llvm.amdgpu.cs.chain intrinsic instead.",
         Call);
 
+  // Disallow passing/returning values with alignment higher than we can
+  // represent.
+  // FIXME: Consider making DataLayout cap the alignment, so this isn't
+  // necessary.
   auto VerifyTypeAlign = [&](Type *Ty, const Twine &Message) {
     if (!Ty->isSized())
       return;
     Align ABIAlign = DL.getABITypeAlign(Ty);
-    Align MaxAlign(ParamMaxAlignment);
-    Check(ABIAlign <= MaxAlign,
+    Check(ABIAlign.value() <= Value::MaximumAlignment,
           "Incorrect alignment of " + Message + " to called function!", Call);
   };
 
diff --git a/llvm/test/Verifier/param-align.ll b/llvm/test/Verifier/param-align.ll
index bfd01cbc9faa5..caa8f9ac41ea5 100644
--- a/llvm/test/Verifier/param-align.ll
+++ b/llvm/test/Verifier/param-align.ll
@@ -2,19 +2,19 @@
 
 ; Large vector for intrinsics is valid
 ; CHECK-NOT: llvm.fshr
-define dso_local <8192 x i32> @test_intrin(<8192 x i32> %l, <8192 x i32> %r, <8192 x i32> %amt) {
+define dso_local <2147483648 x i32> @test_intrin(<2147483648 x i32> %l, <2147483648 x i32> %r, <2147483648 x i32> %amt) {
 entry:
-  %b = call <8192 x i32> @llvm.fshr.v8192i32(<8192 x i32> %l, <8192 x i32> %r, <8192 x i32> %amt)
-  ret <8192 x i32> %b
+  %b = call <2147483648 x i32> @llvm.fshr.v8192i32(<2147483648 x i32> %l, <2147483648 x i32> %r, <2147483648 x i32> %amt)
+  ret <2147483648 x i32> %b
 }
-declare <8192 x i32> @llvm.fshr.v8192i32 (<8192 x i32> %l, <8192 x i32> %r, <8192 x i32> %amt)
+declare <2147483648 x i32> @llvm.fshr.v8192i32 (<2147483648 x i32> %l, <2147483648 x i32> %r, <2147483648 x i32> %amt)
 
 ; CHECK: Incorrect alignment of argument passed to called function!
 ; CHECK: bar
-define dso_local void @foo(<8192 x float> noundef %vec) {
+define dso_local void @foo(<2147483648 x float> noundef %vec) {
 entry:
-  call void @bar(<8192 x float> %vec)
+  call void @bar(<2147483648 x float> %vec)
   ret void
 }
 
-declare dso_local void @bar(<8192 x float>)
+declare dso_local void @bar(<2147483648 x float>)
diff --git a/llvm/test/Verifier/param-attr-align.ll b/llvm/test/Verifier/param-attr-align.ll
index 038bfa3494f89..700efe5376841 100644
--- a/llvm/test/Verifier/param-attr-align.ll
+++ b/llvm/test/Verifier/param-attr-align.ll
@@ -1,9 +1,9 @@
 ; RUN: not llvm-as < %s 2>&1 | FileCheck %s
 
-; CHECK: Attribute 'align' exceed the max size 2^14
+; CHECK: huge alignments are not supported yet
 define dso_local void @foo(ptr %p) {
 entry:
-  call void @bar(ptr noundef byval(<8 x float>) align 32768 %p)
+  call void @bar(ptr noundef byval(<8 x float>) align 8589934592 %p)
   ret void
 }
 
diff --git a/llvm/test/Verifier/param-ret-align.ll b/llvm/test/Verifier/param-ret-align.ll
index dd302c38b53d2..98cbb4ee88a89 100644
--- a/llvm/test/Verifier/param-ret-align.ll
+++ b/llvm/test/Verifier/param-ret-align.ll
@@ -2,19 +2,19 @@
 
 ; Large vector for intrinsics is valid
 ; CHECK-NOT: llvm.fshr
-define dso_local <8192 x i32> @test_intrin(<8192 x i32> %l, <8192 x i32> %r, <8192 x i32> %amt) {
+define dso_local <2147483648 x i32> @test_intrin(<2147483648 x i32> %l, <2147483648 x i32> %r, <2147483648 x i32> %amt) {
 entry:
-  %b = call <8192 x i32> @llvm.fshr.v8192i32(<8192 x i32> %l, <8192 x i32> %r, <8192 x i32> %amt)
-  ret <8192 x i32> %b
+  %b = call <2147483648 x i32> @llvm.fshr.v8192i32(<2147483648 x i32> %l, <2147483648 x i32> %r, <2147483648 x i32> %amt)
+  ret <2147483648 x i32> %b
 }
-declare <8192 x i32> @llvm.fshr.v8192i32 (<8192 x i32> %l, <8192 x i32> %r, <8192 x i32> %amt)
+declare <2147483648 x i32> @llvm.fshr.v2147483648i32 (<2147483648 x i32> %l, <2147483648 x i32> %r, <2147483648 x i32> %amt)
 
 ; CHECK: Incorrect alignment of return type to called function!
 ; CHECK: bar
 define dso_local void @foo() {
 entry:
-  call <8192 x float> @bar()
+  call <2147483648 x float> @bar()
   ret void
 }
 
-declare dso_local <8192 x float> @bar()
+declare dso_local <2147483648 x float> @bar()

nikic · 2024-07-17T07:35:31Z

llvm/lib/IR/Verifier.cpp

      SmallPtrSet<Type *, 4> Visited;
      Check(Attrs.getByValType()->isSized(&Visited),
            "Attribute 'byval' does not support unsized types!", V);
+      Check(DL.getTypeAllocSize(Attrs.getByValType()).getKnownMinValue() <
+                (1ULL << 32),
+            "huge 'byval' arguments are unsupported", V);


Tests for the new checks?

Fznamznon · 2024-07-17T09:46:03Z

Thank you! That helps with the problem I was originally trying to solve with #98629 . Does it make sense to keep #98629 but with the increased limit?

phoebewang · 2024-07-17T12:59:54Z

Thank you! That helps with the problem I was originally trying to solve with #98629 . Does it make sense to keep #98629 but with the increased limit?

Given #98629 has side effect on Darwin targets, I'd like to not pursuit it if the original problem getting solve.

phoebewang

LGTM.

Summary: The 2^14 limit was completely arbitrary; the generic limit is still arbitrary, but at least it's the same arbitrary limit as everything else. While I'm here, also add a verifier check for the ByValOrByRefSize. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251401

… align." Recently we've moved to LLVM 15 on production And on LLVM 15 we've found that this check is no longer valid, the ParamMaxAlignment is too small. ``` llvm/lib/IR/Verifier.cpp auto VerifyTypeAlign = [&](Type *Ty, const Twine &Message) { (...) Align MaxAlign(ParamMaxAlignment); Check(ABIAlign <= MaxAlign, ``` This was already adjusted in the upstream, here: * Backport of llvm/llvm-project#99257

… align." Recently we've moved to LLVM 15 on production And on LLVM 15 we've found that this check is no longer valid, the ParamMaxAlignment is too small. ``` llvm/lib/IR/Verifier.cpp auto VerifyTypeAlign = [&](Type *Ty, const Twine &Message) { (...) Align MaxAlign(ParamMaxAlignment); Check(ABIAlign <= MaxAlign, ``` This was already adjusted in the upstream, here: * Backport of llvm/llvm-project#99257 (cherry picked from commit 77c938c)

Unify max alignment for arguments with generic max align.

47f0345

The 2^14 limit was completely arbitrary; the generic limit is still arbitrary, but at least it's the same arbitrary limit as everything else. While I'm here, also add a verifier check for the ByValOrByRefSize.

efriedma-quic requested review from nikic, phoebewang and Fznamznon July 16, 2024 23:24

llvmbot added the llvm:ir label Jul 16, 2024

nikic reviewed Jul 17, 2024

View reviewed changes

efriedma-quic added 2 commits July 17, 2024 10:36

fixup! add missing test.

f206904

fixup! fix check line

4521773

phoebewang approved these changes Jul 18, 2024

View reviewed changes

efriedma-quic changed the title ~~Unify max alignment for arguments with generic max align.~~ [IR] Unify max alignment for arguments with generic max align. Jul 18, 2024

efriedma-quic merged commit 82cca0c into llvm:main Jul 18, 2024
7 checks passed

Fznamznon mentioned this pull request Jul 19, 2024

[clang] Limit alignment for emitted vectors #98629

Closed

KanRobert mentioned this pull request Aug 13, 2024

[X86,SimplifyCFG] Support hoisting load/store with conditional faulting (Part I) #96878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[IR] Unify max alignment for arguments with generic max align. #99257

[IR] Unify max alignment for arguments with generic max align. #99257

Uh oh!

efriedma-quic commented Jul 16, 2024

Uh oh!

llvmbot commented Jul 16, 2024

Uh oh!

nikic Jul 17, 2024

Uh oh!

Fznamznon commented Jul 17, 2024

Uh oh!

phoebewang commented Jul 17, 2024

Uh oh!

phoebewang left a comment

Uh oh!

Uh oh!

Uh oh!

[IR] Unify max alignment for arguments with generic max align. #99257

[IR] Unify max alignment for arguments with generic max align. #99257

Uh oh!

Conversation

efriedma-quic commented Jul 16, 2024

Uh oh!

llvmbot commented Jul 16, 2024

Uh oh!

nikic Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

Fznamznon commented Jul 17, 2024

Uh oh!

phoebewang commented Jul 17, 2024

Uh oh!

phoebewang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!