[Inliner] Fix bug where attributes are propagated incorrectly #109347

goldsteinn · 2024-09-19T23:07:38Z

[Inliner] Add tests for incorrect propagation of return attrs; NFC
[Inliner] Fix bug where attributes are propagated incorrectly

The bug stems from the fact that we assume the new (inlined) callsite
is calling the same function as the original (callee) callsite. While
this is typically the case, since VMap simplifies the new
instructions, callee intrinsics callsites can end up not corresponding
with the same function.

This can lead to buggy propagation.

The bug stems from the fact that we assume the new (inlined) callsite is calling the same function as the original (callee) callsite. While this is typically the case, since `VMap` simplifies the new instructions, callee intrinsics callsites can end up not corresponding with the same function. This can lead to buggy propagation.

llvmbot · 2024-09-19T23:08:13Z

@llvm/pr-subscribers-llvm-transforms

Author: None (goldsteinn)

Changes

[Inliner] Add tests for incorrect propagation of return attrs; NFC
[Inliner] Fix bug where attributes are propagated incorrectly

The bug stems from the fact that we assume the new (inlined) callsite
is calling the same function as the original (callee) callsite. While
this is typically the case, since VMap simplifies the new
instructions, callee intrinsics callsites can end up not corresponding
with the same function.

This can lead to buggy propagation.

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

3 Files Affected:

(modified) llvm/lib/Transforms/Utils/InlineFunction.cpp (+12)
(modified) llvm/test/Transforms/Inline/access-attributes-prop.ll (+21)
(modified) llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll (+40)

diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 52da65ce01b82b..53e25e53c2a86f 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1383,6 +1383,12 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,
       auto *NewInnerCB = dyn_cast_or_null<CallBase>(VMap.lookup(InnerCB));
       if (!NewInnerCB)
         continue;
+      // The InnerCB might have be simplified during the inlining
+      // process. Only propagate return attributes if we are in fact calling the
+      // same function.
+      if (InnerCB->getCalledFunction() != NewInnerCB->getCalledFunction())
+        continue;
+
       AttributeList AL = NewInnerCB->getAttributes();
       for (unsigned I = 0, E = InnerCB->arg_size(); I < E; ++I) {
         // Check if the underlying value for the parameter is an argument.
@@ -1477,6 +1483,12 @@ static void AddReturnAttributes(CallBase &CB, ValueToValueMapTy &VMap) {
     auto *NewRetVal = dyn_cast_or_null<CallBase>(VMap.lookup(RetVal));
     if (!NewRetVal)
       continue;
+
+    // The RetVal might have be simplified during the inlining
+    // process. Only propagate return attributes if we are in fact calling the
+    // same function.
+    if (RetVal->getCalledFunction() != NewRetVal->getCalledFunction())
+      continue;
     // Backward propagation of attributes to the returned value may be incorrect
     // if it is control flow dependent.
     // Consider:
diff --git a/llvm/test/Transforms/Inline/access-attributes-prop.ll b/llvm/test/Transforms/Inline/access-attributes-prop.ll
index 965f0335b63eab..2c55f5f3b1f6ca 100644
--- a/llvm/test/Transforms/Inline/access-attributes-prop.ll
+++ b/llvm/test/Transforms/Inline/access-attributes-prop.ll
@@ -559,3 +559,24 @@ define void @prop_byval_readonly(ptr %p) {
   call void @foo_byval_readonly(ptr %p)
   ret void
 }
+
+define ptr @caller_bad_param_prop(ptr %p1, ptr %p2, i64 %x) {
+; CHECK-LABEL: define {{[^@]+}}@caller_bad_param_prop
+; CHECK-SAME: (ptr [[P1:%.*]], ptr [[P2:%.*]], i64 [[X:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = call ptr [[P1]](i64 [[X]], ptr [[P2]])
+; CHECK-NEXT:    ret ptr [[TMP1]]
+;
+  %1 = call ptr %p1(i64 %x, ptr %p2)
+  %2 = call ptr @callee_bad_param_prop(ptr %1)
+  ret ptr %2
+}
+
+define ptr @callee_bad_param_prop(ptr readonly %x) {
+; CHECK-LABEL: define {{[^@]+}}@callee_bad_param_prop
+; CHECK-SAME: (ptr readonly [[X:%.*]]) {
+; CHECK-NEXT:    [[R:%.*]] = tail call ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 -1)
+; CHECK-NEXT:    ret ptr [[R]]
+;
+  %r = tail call ptr @llvm.ptrmask(ptr %x, i64 -1)
+  ret ptr %r
+}
diff --git a/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll b/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
index dc685d2c4d1368..930bef43df1dba 100644
--- a/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
+++ b/llvm/test/Transforms/Inline/ret_attr_align_and_noundef.ll
@@ -421,3 +421,43 @@ define i8 @caller16_not_intersecting_ranges() {
   call void @use.val(i8 %r)
   ret i8 %r
 }
+
+
+define ptr @caller_bad_ret_prop(ptr %p1, ptr %p2, i64 %x, ptr %other) {
+; CHECK-LABEL: define ptr @caller_bad_ret_prop
+; CHECK-SAME: (ptr [[P1:%.*]], ptr [[P2:%.*]], i64 [[X:%.*]], ptr [[OTHER:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = call noundef ptr [[P1]](i64 [[X]], ptr [[P2]])
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[TMP1]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[T_I:%.*]], label [[F_I:%.*]]
+; CHECK:       T.i:
+; CHECK-NEXT:    br label [[CALLEE_BAD_RET_PROP_EXIT:%.*]]
+; CHECK:       F.i:
+; CHECK-NEXT:    br label [[CALLEE_BAD_RET_PROP_EXIT]]
+; CHECK:       callee_bad_ret_prop.exit:
+; CHECK-NEXT:    [[TMP2:%.*]] = phi ptr [ [[OTHER]], [[T_I]] ], [ [[TMP1]], [[F_I]] ]
+; CHECK-NEXT:    ret ptr [[TMP2]]
+;
+  %1 = call noundef ptr %p1(i64 %x, ptr %p2)
+  %2 = call nonnull ptr @callee_bad_ret_prop(ptr %1, ptr %other)
+  ret ptr %2
+}
+
+define ptr @callee_bad_ret_prop(ptr %x, ptr %other) {
+; CHECK-LABEL: define ptr @callee_bad_ret_prop
+; CHECK-SAME: (ptr [[X:%.*]], ptr [[OTHER:%.*]]) {
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq ptr [[X]], null
+; CHECK-NEXT:    br i1 [[CMP]], label [[T:%.*]], label [[F:%.*]]
+; CHECK:       T:
+; CHECK-NEXT:    ret ptr [[OTHER]]
+; CHECK:       F:
+; CHECK-NEXT:    [[R:%.*]] = tail call ptr @llvm.ptrmask.p0.i64(ptr [[X]], i64 -1)
+; CHECK-NEXT:    ret ptr [[R]]
+;
+  %cmp = icmp eq ptr %x, null
+  br i1 %cmp, label %T, label %F
+T:
+  ret ptr %other
+F:
+  %r = tail call ptr @llvm.ptrmask(ptr %x, i64 -1)
+  ret ptr %r
+}

goldsteinn · 2024-09-19T23:09:12Z

NB: this (or any other fix we settle on) will need to be backported to release/19.

efriedma-quic · 2024-09-19T23:19:29Z

llvm/lib/Transforms/Utils/InlineFunction.cpp

+    // The RetVal might have be simplified during the inlining
+    // process. Only propagate return attributes if we are in fact calling the
+    // same function.
+    if (RetVal->getCalledFunction() != NewRetVal->getCalledFunction())


This doesn't seem like it will reliably detect the issue.

First off, if both calls are indirect, both calls to getCalledFunction() would return nullptr, in which case you aren't getting any useful information at all.

Second, even if both calls are calling the "same" function, it might not have the same behavior if the arguments are different. Or global state is different. Not sure how likely this is given the limited simplifications the inline performs, but it's hard to rule out.

So I think we need to avoid using VMap, and pass down the equivalence information some other way.

This doesn't seem like it will reliably detect the issue.

First off, if both calls are indirect, both calls to getCalledFunction() would return nullptr, in which case you aren't getting any useful information at all.

Fair, I think its fine to just disable all prop for indirect functions.

Second, even if both calls are calling the "same" function, it might not have the same behavior if the arguments are different. Or global state is different. Not sure how likely this is given the limited simplifications the inline performs, but it's hard to rule out.

So I think we need to avoid using VMap, and pass down the equivalence information some other way.

Is that really a valid concern? We don't allow arbitrary transforms on the new instructions. Within the confines of what we do do, I don't see how we could change global state or produce and argument that is not semantically equiv to what was in the original.

That being said, we could also move the propagation code to the function cloning logic before simplifications/etc.

Suppose you inline a function that contains two calls to foo(). The second call gets simplified away, and the simplified value is the first call to foo(). So the vmap lookup finds the first call to foo(), which has completely different arguments/state.

You are worried about something like:

declare ptr @foo(ptr) memory(none) willreturn nounwind define ptr @caller(ptr %p, i1 %c) { %r = call ptr @callee(ptr %p, i1 %c) ret ptr %r } define ptr @callee(ptr %p, i1 %c) { %x = call ptr @foo(ptr %p) br i1 %c, label %T, label %F T: ret ptr %x F: %r2 = call nonnull ptr @foo(ptr %p) ret ptr %r2 }

?

Where we might prop the nonnull from the second call to the first call if it got simplified?

I guess the other options are either move this to before simplifications in the cloneFunction call or track which callbases get simplified and skip them.

Or we could just disable the simplifyInstruction on callbases

Personally I'm for just not using simplifyInstruction on callbases in CloneFunction in inliner. Don't see much value in it in a realistic pipeline and unless I'm missing something, I think it solves the bug.
But would like to hear if anyone else has an opinion on how this should be solved.

nikic

This problem sounds familiar :) The right way to handle this is to query InlinedFunctionInfo.isSimplified().

goldsteinn · 2024-09-20T14:10:45Z

This problem sounds familiar :) The right way to handle this is to query InlinedFunctionInfo.isSimplified().

That is much more convenient :)

nikic

LGTM

goldsteinn · 2024-09-21T00:58:29Z

/cherry-pick a9352a0

llvmbot · 2024-09-21T01:03:28Z

Failed to cherry-pick: a9352a0

https://github.com/llvm/llvm-project/actions/runs/10968306432

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

goldsteinn · 2024-09-21T01:33:27Z

/cherry-pick a9352a0

llvmbot · 2024-09-21T01:38:29Z

Failed to cherry-pick: a9352a0

https://github.com/llvm/llvm-project/actions/runs/10968564268

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

…09347) - **[Inliner] Add tests for incorrect propagation of return attrs; NFC** - **[Inliner] Fix bug where attributes are propagated incorrectly** The bug stems from the fact that we assume the new (inlined) callsite is calling the same function as the original (callee) callsite. While this is typically the case, since `VMap` simplifies the new instructions, callee intrinsics callsites can end up not corresponding with the same function. This can lead to buggy propagation. (cherry picked from commit a9352a0)

…09347) - **[Inliner] Add tests for incorrect propagation of return attrs; NFC** - **[Inliner] Fix bug where attributes are propagated incorrectly** The bug stems from the fact that we assume the new (inlined) callsite is calling the same function as the original (callee) callsite. While this is typically the case, since `VMap` simplifies the new instructions, callee intrinsics callsites can end up not corresponding with the same function. This can lead to buggy propagation.

goldsteinn added 2 commits September 19, 2024 18:04

[Inliner] Add tests for incorrect propagation of return attrs; NFC

998f99e

llvmbot added the llvm:transforms label Sep 19, 2024

goldsteinn changed the title ~~goldsteinn/fix inliner new old cb desync~~ [Inliner] Fix bug where attributes are propagated incorrectly Sep 19, 2024

goldsteinn requested review from nikic, andjo403 and dtcxzyw September 19, 2024 23:08

goldsteinn mentioned this pull request Sep 19, 2024

[Inliner] Propagate more attributes to params when inlining #91101

Merged

efriedma-quic reviewed Sep 19, 2024

View reviewed changes

nikic requested changes Sep 20, 2024

View reviewed changes

Use InlinedFunctionInfo.isSimplified

6536bc4

nikic approved these changes Sep 20, 2024

View reviewed changes

goldsteinn merged commit a9352a0 into llvm:main Sep 21, 2024
8 checks passed

goldsteinn added this to the LLVM 19.X Release milestone Sep 21, 2024

llvmbot added the release:cherry-pick-failed label Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Inliner] Fix bug where attributes are propagated incorrectly #109347

[Inliner] Fix bug where attributes are propagated incorrectly #109347

goldsteinn commented Sep 19, 2024 •

edited

Loading

llvmbot commented Sep 19, 2024

goldsteinn commented Sep 19, 2024

efriedma-quic Sep 19, 2024

goldsteinn Sep 20, 2024

efriedma-quic Sep 20, 2024

goldsteinn Sep 20, 2024

goldsteinn Sep 20, 2024

goldsteinn Sep 20, 2024

goldsteinn Sep 20, 2024

nikic left a comment

goldsteinn commented Sep 20, 2024

nikic left a comment

goldsteinn commented Sep 21, 2024

llvmbot commented Sep 21, 2024

goldsteinn commented Sep 21, 2024

llvmbot commented Sep 21, 2024

[Inliner] Fix bug where attributes are propagated incorrectly #109347

[Inliner] Fix bug where attributes are propagated incorrectly #109347

Conversation

goldsteinn commented Sep 19, 2024 • edited Loading

llvmbot commented Sep 19, 2024

goldsteinn commented Sep 19, 2024

efriedma-quic Sep 19, 2024

Choose a reason for hiding this comment

goldsteinn Sep 20, 2024

Choose a reason for hiding this comment

efriedma-quic Sep 20, 2024

Choose a reason for hiding this comment

goldsteinn Sep 20, 2024

Choose a reason for hiding this comment

goldsteinn Sep 20, 2024

Choose a reason for hiding this comment

goldsteinn Sep 20, 2024

Choose a reason for hiding this comment

goldsteinn Sep 20, 2024

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

goldsteinn commented Sep 20, 2024

nikic left a comment

Choose a reason for hiding this comment

goldsteinn commented Sep 21, 2024

llvmbot commented Sep 21, 2024

goldsteinn commented Sep 21, 2024

llvmbot commented Sep 21, 2024

goldsteinn commented Sep 19, 2024 •

edited

Loading