[ArgPromotion] Remove redundant logic from recursive argpromotion code #98657

vedantparanjape-amd · 2024-07-12T16:24:08Z

This patch further cleans up the implementation by removing some redundant checks and replacing cast<> with get() calls.

This contribution is based on the discussion in #78735

llvmbot · 2024-07-12T16:24:39Z

@llvm/pr-subscribers-llvm-transforms

Author: Vedant Paranjape (vedantparanjape-amd)

Changes

This patch further cleans up the implementation by removing some redundant checks and replacing cast<> with get() calls. It adds a check to see if function call type matches the function type.

This contribution is based on the discussion in #78735

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

2 Files Affected:

(modified) llvm/lib/Transforms/IPO/ArgumentPromotion.cpp (+5-3)
(added) llvm/test/Transforms/ArgumentPromotion/recursion/recursion-diff-call-types.ll (+68)

diff --git a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
index 77dbf349df0df..78805f7fc9554 100644
--- a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
+++ b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
@@ -640,8 +640,10 @@ static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
     }
 
     auto *CB = dyn_cast<CallBase>(V);
-    Value *PtrArg = cast<Value>(U);
-    if (CB && PtrArg && CB->getCalledFunction() == CB->getFunction()) {
+    Value *PtrArg = U->get();
+    if (CB && CB->getCalledFunction() == CB->getFunction() &&
+        CB->getCalledFunction()->getReturnType() ==
+            CB->getFunction()->getReturnType()) {
       if (PtrArg != Arg) {
         LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
                           << "pointer offset is not equal to zero\n");
@@ -649,7 +651,7 @@ static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
       }
 
       unsigned int ArgNo = Arg->getArgNo();
-      if (CB->getArgOperand(ArgNo) != Arg || U->getOperandNo() != ArgNo) {
+      if (U->getOperandNo() != ArgNo) {
         LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
                           << "arg position is different in callee\n");
         return false;
diff --git a/llvm/test/Transforms/ArgumentPromotion/recursion/recursion-diff-call-types.ll b/llvm/test/Transforms/ArgumentPromotion/recursion/recursion-diff-call-types.ll
new file mode 100644
index 0000000000000..a4ee73727108a
--- /dev/null
+++ b/llvm/test/Transforms/ArgumentPromotion/recursion/recursion-diff-call-types.ll
@@ -0,0 +1,68 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=argpromotion < %s | FileCheck %s
+define internal i32 @foo(ptr %x, i32 %n, i32 %m) {
+; CHECK-LABEL: define internal i32 @foo(
+; CHECK-SAME: ptr [[X:%.*]], i32 [[N:%.*]], i32 [[M:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i32 [[N]], 0
+; CHECK-NEXT:    br i1 [[CMP]], label %[[COND_TRUE:.*]], label %[[COND_FALSE:.*]]
+; CHECK:       [[COND_TRUE]]:
+; CHECK-NEXT:    [[VAL:%.*]] = load i32, ptr [[X]], align 4
+; CHECK-NEXT:    br label %[[RETURN:.*]]
+; CHECK:       [[COND_FALSE]]:
+; CHECK-NEXT:    [[VAL2:%.*]] = load i32, ptr [[X]], align 4
+; CHECK-NEXT:    [[SUBVAL:%.*]] = sub i32 [[N]], 1
+; CHECK-NEXT:    [[CALLRET0:%.*]] = call float @foo(ptr [[X]], i32 [[SUBVAL]], i32 [[VAL2]])
+; CHECK-NEXT:    [[CALLRET1:%.*]] = call i32 @foo(ptr [[X]], i32 [[SUBVAL]], i32 [[VAL2]])
+; CHECK-NEXT:    [[SUBVAL2:%.*]] = sub i32 [[N]], 2
+; CHECK-NEXT:    [[CALLRET2:%.*]] = call i32 @foo(ptr [[X]], i32 [[SUBVAL2]], i32 [[M]])
+; CHECK-NEXT:    [[CMP2:%.*]] = add i32 [[CALLRET1]], [[CALLRET2]]
+; CHECK-NEXT:    br label %[[RETURN]]
+; CHECK:       [[COND_NEXT:.*]]:
+; CHECK-NEXT:    br label %[[RETURN]]
+; CHECK:       [[RETURN]]:
+; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi i32 [ [[VAL]], %[[COND_TRUE]] ], [ [[CMP2]], %[[COND_FALSE]] ], [ poison, %[[COND_NEXT]] ]
+; CHECK-NEXT:    ret i32 [[RETVAL_0]]
+;
+entry:
+  %cmp = icmp ne i32 %n, 0
+  br i1 %cmp, label %cond_true, label %cond_false
+
+cond_true:                                        ; preds = %entry
+  %val = load i32, ptr %x, align 4
+  br label %return
+
+cond_false:                                       ; preds = %entry
+  %val2 = load i32, ptr %x, align 4
+  %subval = sub i32 %n, 1
+  %callret0 = call float @foo(ptr %x, i32 %subval, i32 %val2)
+  %callret1 = call i32 @foo(ptr %x, i32 %subval, i32 %val2)
+  %subval2 = sub i32 %n, 2
+  %callret2 = call i32 @foo(ptr %x, i32 %subval2, i32 %m)
+  %cmp2 = add i32 %callret1, %callret2
+  br label %return
+
+cond_next:                                        ; No predecessors!
+  br label %return
+
+return:                                           ; preds = %cond_next, %cond_false, %cond_true
+  %retval.0 = phi i32 [ %val, %cond_true ], [ %cmp2, %cond_false ], [ poison, %cond_next ]
+  ret i32 %retval.0
+}
+
+define i32 @bar(ptr align(4) dereferenceable(4) %x, i32 %n, i32 %m) {
+; CHECK-LABEL: define i32 @bar(
+; CHECK-SAME: ptr align 4 dereferenceable(4) [[X:%.*]], i32 [[N:%.*]], i32 [[M:%.*]]) {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[CALLRET3:%.*]] = call i32 @foo(ptr [[X]], i32 [[N]], i32 [[M]])
+; CHECK-NEXT:    br label %[[RETURN:.*]]
+; CHECK:       [[RETURN]]:
+; CHECK-NEXT:    ret i32 [[CALLRET3]]
+;
+entry:
+  %callret3 = call i32 @foo(ptr %x, i32 %n, i32 %m)
+  br label %return
+
+return:                                           ; preds = %entry
+  ret i32 %callret3
+}

vedantparanjape-amd · 2024-07-12T16:26:34Z

@nikic @arsenm I have created this PR to address the changes discussed in the PR #78735

arsenm · 2024-07-12T16:46:37Z

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

      if (PtrArg != Arg) {
        LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
                          << "pointer offset is not equal to zero\n");
        return false;
      }

      unsigned int ArgNo = Arg->getArgNo();
-      if (CB->getArgOperand(ArgNo) != Arg || U->getOperandNo() != ArgNo) {
+      if (U->getOperandNo() != ArgNo) {


Can you use isCallee?

I don't think so, as I need to check the position of the argument in the call matches the position in the caller function.

arsenm · 2024-07-12T16:47:14Z

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

+        CB->getCalledFunction()->getReturnType() ==
+            CB->getFunction()->getReturnType()) {


Why is the function return type important? getCalledFunction is supposed to return null if the function types mismatch already?

I am confused, wasn't I meant to add the check to see if the function call type doesn't match the function def?

Ah sorry, I forgot that getCalledFunction already checks the function type. Ignore my comment about that.

okay, cool! so I can remove that check ? and then can I get a LGTM.

This patch further cleans up the implementation by removing some redundant checks and replacing cast<> with get() calls.

nikic

LGTM

llvm-ci · 2024-07-14T05:14:58Z

LLVM Buildbot has detected a new failure on builder clang-armv7-global-isel running on linaro-clang-armv7-global-isel while building llvm at step 12 "setup lit".

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

Here is the relevant piece of the build log for the reference:

Step 12 (setup lit) failure: '/home/tcwg-buildbot/worker/clang-armv7-global-isel/test/sandbox/bin/python /home/tcwg-buildbot/worker/clang-armv7-global-isel/test/lnt/setup.py ...' (failure)
running develop
running egg_info
writing LNT.egg-info/PKG-INFO
writing dependency_links to LNT.egg-info/dependency_links.txt
writing entry points to LNT.egg-info/entry_points.txt
writing requirements to LNT.egg-info/requires.txt
writing top-level names to LNT.egg-info/top_level.txt
reading manifest file 'LNT.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
no previously-included directories found matching 'docs/_build'
no previously-included directories found matching 'tests/*/Output'
no previously-included directories found matching 'tests/*/*/Output'
writing manifest file 'LNT.egg-info/SOURCES.txt'
running build_ext
copying build/lib.linux-armv8l-3.8/lnt/testing/profile/cPerf.cpython-38-arm-linux-gnueabihf.so -> lnt/testing/profile
Creating /home/tcwg-buildbot/worker/clang-armv7-global-isel/test/sandbox/lib/python3.8/site-packages/LNT.egg-link (link to .)
Adding LNT 0.4.2.dev0 to easy-install.pth file
Installing lnt script to /home/tcwg-buildbot/worker/clang-armv7-global-isel/test/sandbox/bin

Installed /home/tcwg-buildbot/worker/clang-armv7-global-isel/test/lnt
Processing dependencies for LNT==0.4.2.dev0
Searching for typing
Reading https://pypi.org/simple/typing/
Download error on https://pypi.org/simple/typing/: [Errno -3] Temporary failure in name resolution -- Some packages may not be found!
Couldn't find index page for 'typing' (maybe misspelled?)
Scanning index of all packages (this may take a while)
Reading https://pypi.org/simple/
Download error on https://pypi.org/simple/: [Errno -3] Temporary failure in name resolution -- Some packages may not be found!
No local packages or working download links found for typing
error: Could not find suitable distribution for Requirement.parse('typing')

llvm#98657) This patch further cleans up the implementation by removing some redundant checks and replacing cast<> with get() calls. This contribution is based on the discussion in llvm#78735

llvmbot added the llvm:transforms label Jul 12, 2024

vedantparanjape-amd requested review from nikic, efriedma-quic and arsenm and removed request for efriedma-quic July 12, 2024 16:26

vedantparanjape-amd mentioned this pull request Jul 12, 2024

[ArgPromotion] Handle pointer arguments of recursive calls #78735

Merged

arsenm reviewed Jul 12, 2024

View reviewed changes

vedantparanjape-amd force-pushed the temp branch from db01c69 to fb27f6f Compare July 12, 2024 18:14

[ArgPromotion] Remove redundant logic from recursive argpromotion code

5110a86

This patch further cleans up the implementation by removing some redundant checks and replacing cast<> with get() calls.

vedantparanjape-amd force-pushed the temp branch from 36a7d65 to 5110a86 Compare July 12, 2024 18:16

nikic approved these changes Jul 13, 2024

View reviewed changes

vedantparanjape-amd merged commit 78266ab into llvm:main Jul 14, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ArgPromotion] Remove redundant logic from recursive argpromotion code #98657

[ArgPromotion] Remove redundant logic from recursive argpromotion code #98657

vedantparanjape-amd commented Jul 12, 2024 •

edited

Loading

llvmbot commented Jul 12, 2024

vedantparanjape-amd commented Jul 12, 2024

arsenm Jul 12, 2024

vedantparanjape-amd Jul 12, 2024

arsenm Jul 12, 2024

vedantparanjape-amd Jul 12, 2024

nikic Jul 12, 2024

vedantparanjape-amd Jul 12, 2024

nikic left a comment

llvm-ci commented Jul 14, 2024

		CB->getCalledFunction()->getReturnType() ==
		CB->getFunction()->getReturnType()) {

[ArgPromotion] Remove redundant logic from recursive argpromotion code #98657

[ArgPromotion] Remove redundant logic from recursive argpromotion code #98657

Conversation

vedantparanjape-amd commented Jul 12, 2024 • edited Loading

llvmbot commented Jul 12, 2024

vedantparanjape-amd commented Jul 12, 2024

arsenm Jul 12, 2024

Choose a reason for hiding this comment

vedantparanjape-amd Jul 12, 2024

Choose a reason for hiding this comment

arsenm Jul 12, 2024

Choose a reason for hiding this comment

vedantparanjape-amd Jul 12, 2024

Choose a reason for hiding this comment

nikic Jul 12, 2024

Choose a reason for hiding this comment

vedantparanjape-amd Jul 12, 2024

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

llvm-ci commented Jul 14, 2024

vedantparanjape-amd commented Jul 12, 2024 •

edited

Loading