Skip to content

[Inliner] Prevent adding pointer attributes to non-pointer arguments #115569

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

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Nov 8, 2024

Fixes a crash seen after #114311

@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Harald van Dijk (hvdijk)

Changes

Fixes a crash seen after #114311


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/InlineFunction.cpp (+9)
  • (modified) llvm/test/Transforms/Inline/arg-attr-propagation.ll (+26)
diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index a27cb4dd219c30..cbad66ed9dfe5e 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1477,6 +1477,15 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,
 
         // If so, propagate its access attributes.
         AL = AL.addParamAttributes(Context, I, ValidObjParamAttrs[ArgNo]);
+
+        // If the argument is not a pointer type, remove attributes which only
+        // apply to pointer types.
+        if (!NewInnerCB->getArgOperand(I)->getType()->isPointerTy()) {
+          AL = AL.removeParamAttribute(Context, I, Attribute::ReadNone);
+          AL = AL.removeParamAttribute(Context, I, Attribute::ReadOnly);
+          continue;
+        }
+
         // We can have conflicting attributes from the inner callsite and
         // to-be-inlined callsite. In that case, choose the most
         // restrictive.
diff --git a/llvm/test/Transforms/Inline/arg-attr-propagation.ll b/llvm/test/Transforms/Inline/arg-attr-propagation.ll
index 7b096539e7e1b1..fad6c7ced2edd2 100644
--- a/llvm/test/Transforms/Inline/arg-attr-propagation.ll
+++ b/llvm/test/Transforms/Inline/arg-attr-propagation.ll
@@ -76,3 +76,29 @@ define i32 @caller3(ptr dereferenceable(33) %t1) {
   ret i32 %t2
 }
 
+; Make sure that we don't propagate a pointer-only attribute to a vector of pointers.
+
+declare void @helper4(<4 x ptr> %ptr)
+
+define void @callee4(ptr readonly %ptr, <4 x i64> %idx) {
+; CHECK-LABEL: define {{[^@]+}}@callee4
+; CHECK-SAME: (ptr readonly [[PTR:%.*]], <4 x i64> [[IDX:%.*]]) {
+; CHECK-NEXT:    [[PTRS:%.*]] = getelementptr inbounds i8, ptr [[PTR]], <4 x i64> [[IDX]]
+; CHECK-NEXT:    call void @helper4(<4 x ptr> [[PTRS]])
+; CHECK-NEXT:    ret void
+;
+  %ptrs = getelementptr inbounds i8, ptr %ptr, <4 x i64> %idx
+  call void @helper4(<4 x ptr> %ptrs)
+  ret void
+}
+
+define void @caller4(ptr readonly %ptr, <4 x i64> %idx) {
+; CHECK-LABEL: define {{[^@]+}}@caller4
+; CHECK-SAME: (ptr readonly [[PTR:%.*]], <4 x i64> [[IDX:%.*]]) {
+; CHECK-NEXT:    [[PTRS_I:%.*]] = getelementptr inbounds i8, ptr [[PTR]], <4 x i64> [[IDX]]
+; CHECK-NEXT:    call void @helper4(<4 x ptr> [[PTRS_I]])
+; CHECK-NEXT:    ret void
+;
+  call void @callee4(ptr readonly %ptr, <4 x i64> %idx)
+  ret void
+}

@dtcxzyw dtcxzyw requested review from nikic and goldsteinn November 9, 2024 01:31
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.

We should entirely skip the getUnderlyingObject logic for non-pointer arguments instead.

@hvdijk hvdijk force-pushed the avoid-readonly-vectors branch from 48f6814 to 199420a Compare November 9, 2024 13:30
@hvdijk
Copy link
Contributor Author

hvdijk commented Nov 9, 2024

We should entirely skip the getUnderlyingObject logic for non-pointer arguments instead.

Good thinking, that passes testing and is simpler. Done.

@hvdijk hvdijk requested a review from nikic November 9, 2024 14:17
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.

LGTM

@hvdijk hvdijk merged commit ccaded2 into llvm:main Nov 9, 2024
8 checks passed
@hvdijk hvdijk deleted the avoid-readonly-vectors branch November 9, 2024 16:17
@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 9, 2024

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/4437

Here is the relevant piece of the build log for the reference
Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************


Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 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.

5 participants