[ArgPromotion] Handle pointer arguments of recursive calls #78735

vedantparanjape-amd · 2024-01-19T15:53:18Z

Argument promotion doesn't handle recursive function calls to promote arguments. This patch adds functionality to handle self recursive function calls, i.e. whose SCC size is 1. Due to complexity of ValueTracking in recursive calls with SCC size greater than 1, we bail out in such cases

Fixes #1259

llvmbot · 2024-01-19T15:53:51Z

@llvm/pr-subscribers-llvm-transforms

Author: Vedant Paranjape (vedantparanjape-amd)

Changes

Tries to fix llvm#1259. This implementation fails pr42028-recursion.ll, the compiler crashes as it is not able to handle call graph as follows:

A -&gt; B -&gt; C
         ↑____|

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

1 Files Affected:

(modified) llvm/lib/Transforms/IPO/ArgumentPromotion.cpp (+54-1)

diff --git a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
index 8058282c422503..072a8c46f1e54c 100644
--- a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
+++ b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
@@ -609,13 +609,66 @@ static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
       // unknown users
     }
 
+    auto *CI = dyn_cast<CallInst>(V);
+    if (IsRecursive && CI && CI->getFunction() == Arg->getParent()) {
+      dbgs() << "Found recursive call\n";
+      Type *Ty = CI->getType();
+      Value *Ptr = CI->getArgOperand(U->getOperandNo());
+      Align PtrAlign = Ptr->getPointerAlignment(DL);
+      APInt Offset(DL.getIndexTypeSizeInBits(Ptr->getType()), 0);
+      Ptr = Ptr->stripAndAccumulateConstantOffsets(DL, Offset, /* AllowNonInbounds */ true);
+      if (Ptr != Arg)
+        return false;
+
+      if (Offset.getSignificantBits() >= 64)
+        return false;
+
+      TypeSize Size = DL.getTypeStoreSize(Ty);
+      // Don't try to promote scalable types.
+      if (Size.isScalable())
+        return false;
+
+      int64_t Off = Offset.getSExtValue();
+      auto Pair = ArgParts.try_emplace(
+          Off, ArgPart{Ty, PtrAlign, nullptr});
+      ArgPart &Part = Pair.first->second;
+
+      // We limit promotion to only promoting up to a fixed number of elements of
+      // the aggregate.
+      if (MaxElements > 0 && ArgParts.size() > MaxElements) {
+        LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
+                          << "more than " << MaxElements << " parts\n");
+        return false;
+      }
+
+      Part.Alignment = std::max(Part.Alignment, PtrAlign);
+      continue;
+    }
     // Unknown user.
     LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
                       << "unknown user " << *V << "\n");
     return false;
   }
 
-  if (NeededDerefBytes || NeededAlign > 1) {
+  // Incase of functions with recursive calls, this check will fail when it
+  // tries to look at the first caller of this function. The caller may or may
+  // not have a load, incase it doesn't load the pointer being passed, this
+  // check will fail. So, it's safe to skip the check incase we know that we
+  // are dealing with a recursive call.
+  //
+  // def fun(ptr %a) {
+  //   ...
+  //   %loadres = load i32, ptr %a, align 4
+  //   %res = call i32 @fun(ptr %a)
+  //   ...
+  // }
+  //
+  // def bar(ptr %x) {
+  //   ...
+  //   %resbar = call i32 @fun(ptr %x)
+  //   ...
+  // }
+  if (!IsRecursive && (NeededDerefBytes || NeededAlign > 1)) {
     // Try to prove a required deref / aligned requirement.
     if (!allCallersPassValidPointerForArgument(Arg, NeededAlign,
                                                NeededDerefBytes)) {

github-actions · 2024-01-19T15:55:42Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jdoerfert

I haven't looked at this code in a long time. Left some comments though.

Tests missing.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

arsenm

Reverse ping

vedantparanjape-amd · 2024-05-31T11:33:25Z

@arsenm @jdoerfert I have addressed the comment, but I am stuck at one thing. The testcase: Transforms/ArgumentPromotion/pr42028-recursion.ll

A -> B -> C
     ↑____|

It has a SCC like this. Somehow, not able to handle the case with call instructions.

arsenm · 2024-05-31T11:36:09Z

@arsenm @jdoerfert I have addressed the comment, but I am stuck at one thing. The testcase: Transforms/ArgumentPromotion/pr42028-recursion.ll
A -> B -> C
     ↑____|
It has a SCC like this. Somehow, not able to handle the case with call instructions.

Handle in what way? It breaks, or is sub-optimal? If it's not broken, it doesn't need to be perfect in the first attempt

vedantparanjape-amd · 2024-05-31T12:49:33Z

@arsenm @jdoerfert I have addressed the comment, but I am stuck at one thing. The testcase: Transforms/ArgumentPromotion/pr42028-recursion.ll
A -> B -> C
     ↑____|
It has a SCC like this. Somehow, not able to handle the case with call instructions.
Handle in what way? It breaks, or is sub-optimal? If it's not broken, it doesn't need to be perfect in the first attempt

It breaks (SE), while handling that case, if we try to do argument promotion for such a case it will lead to recursion. Anyways when we try to handle such a case, it asserts at isAllocaPromotable() check, because we cannot promote Allocas which are a ptr, as we don't know the depth of pointer nesting.

arsenm · 2024-05-31T12:52:07Z

@arsenm @jdoerfert I have addressed the comment,

This PR hasn't been updated?

vedantparanjape-amd · 2024-05-31T12:53:00Z

@arsenm @jdoerfert I have addressed the comment,

This PR hasn't been updated?

Just a min, cleaning up and pushing.

vedantparanjape-amd · 2024-05-31T12:59:04Z

@arsenm @jdoerfert I have addressed the comment,

This PR hasn't been updated?

Done, please take a look! adding the testcases in sometime.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

arsenm · 2024-05-31T13:13:26Z

Done, please take a look! adding the testcases in sometime.

The test is the most important part of the review

vedantparanjape-amd · 2024-05-31T13:55:10Z

Done, please take a look! adding the testcases in sometime.

The test is the most important part of the review

I have added the testcase as well, and addressed the comments best I could.

vedantparanjape-amd · 2024-06-03T08:28:42Z

@arsenm implemented a check on function call args to see if they are being dereferenced somewhere above the call. If they are not we can bail out, and not promote the argument. This will remove the need for IsSelfRecursive flag, and provide a better way to detect recursive arg promotions.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

Argument promotion doesn't handle recursive function calls to promote arguments. This patch adds functionality to handle self recursive function calls, i.e. whose SCC size is 1. Due to complexity of Value Tracking in recursive calls with SCC size greater than 1, we bail out in such cases.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

nikic · 2024-07-12T08:23:56Z

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

@@ -611,6 +639,33 @@ static bool findArgParts(Argument *Arg, const DataLayout &DL, AAResults &AAR,
      // unknown users
    }

+    auto *CB = dyn_cast<CallBase>(V);
+    Value *PtrArg = cast<Value>(U);
+    if (CB && PtrArg && CB->getCalledFunction() == CB->getFunction()) {


This should probably also check that the function type of the call and function match? I wouldn't want to reason about how the arguments map if this is not the case.

What do you mean by function type, is it the thing returned by getFunctionType() ? I am not able to understand why it would be different ? Can you give an example.

The call and the function have separate types that do not match:

declare float @ret_float() define i32 @call_as_i32() { %val = call i32 @ret_float() ret i32 %val }

The call and the function have separate types that do not match:

declare float @ret_float() define i32 @call_as_i32() { %val = call i32 @ret_float() ret i32 %val }

I didn't know this was possible, I mean it should not be valid? Okay, I will check for the return types as well.

created a new PR to address these changes, please take a look (#98657)

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

#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 #78735

Argument promotion doesn't handle recursive function calls to promote arguments. This patch adds functionality to handle self recursive function calls, i.e. whose SCC size is 1. Due to complexity of ValueTracking in recursive calls with SCC size greater than 1, we bail out in such cases.

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 Jan 19, 2024

vedantparanjape-amd requested review from nikic and arsenm January 19, 2024 15:55

vedantparanjape-amd force-pushed the 1259 branch from 013ead7 to 9a6065d Compare January 19, 2024 15:57

vedantparanjape-amd requested review from jdoerfert and Endilll and removed request for nikic January 19, 2024 15:58

Endilll removed their request for review January 19, 2024 16:24

jdoerfert reviewed Jan 22, 2024

View reviewed changes

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp Outdated Show resolved Hide resolved

arsenm reviewed May 8, 2024

View reviewed changes

vedantparanjape-amd force-pushed the 1259 branch from 9a6065d to b8b89bb Compare May 31, 2024 12:57

arsenm reviewed May 31, 2024

View reviewed changes

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp Show resolved Hide resolved

vedantparanjape-amd force-pushed the 1259 branch from b8b89bb to fbc9cad Compare May 31, 2024 13:53

vedantparanjape-amd force-pushed the 1259 branch from fbc9cad to 1301ba4 Compare June 3, 2024 08:26

arsenm added the ipo Interprocedural optimizations label Jun 6, 2024

arsenm reviewed Jun 6, 2024

View reviewed changes

vedantparanjape-amd requested review from daniel-grumberg, rupprecht and keith as code owners June 7, 2024 02:59

vedantparanjape-amd added 19 commits July 11, 2024 18:40

Add a check to stop recursive promotion of ptr args

3f7d632

Address review comments

d3d6872

Address review 2 comments

ba01424

minor nitpick

a183702

Address review 3 comments

d5bbcf0

Address review 4 comments

b7c4129

Refactor and add comments, also address review comments

979dd21

Add check for self recursion

4457b7a

Add check if call is recursion

d9ca124

remove isSelfRecursive check

169eee2

Add check for argument number as well in recursive calls

9dc8504

Added more testcases

fb7b1ad

Add more tests, and move them to a folder

42fa56e

Added testcase for multiple elements

78f9df5

Removed IsSelfRecursive from findArgParts

d0968a5

Use SmallPtrSetImpl in a helper function

213c134

minor fix to SmallPtrSetImpl

881c4cd

Remove use of stripAndAccumulateConstantOffsets

5a053f9

vedantparanjape-amd force-pushed the 1259 branch from 2644a23 to 5a053f9 Compare July 11, 2024 13:11

vedantparanjape-amd removed the request for review from nikic July 11, 2024 13:37

vedantparanjape-amd merged commit 10df988 into llvm:main Jul 12, 2024
7 checks passed

vedantparanjape-amd mentioned this pull request Jul 12, 2024

Argument Promotion needs to be smarter #1259

Closed

MaskRay added a commit that referenced this pull request Jul 12, 2024

[ArgumentPromotion] Fix -Wunused-but-set-variable after #78735

9c92276

nikic reviewed Jul 12, 2024

View reviewed changes

vedantparanjape-amd mentioned this pull request Jul 12, 2024

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

Merged

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024

[ArgumentPromotion] Fix -Wunused-but-set-variable after llvm#78735

e38bbbd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ArgPromotion] Handle pointer arguments of recursive calls #78735

[ArgPromotion] Handle pointer arguments of recursive calls #78735

vedantparanjape-amd commented Jan 19, 2024 •

edited

Loading

llvmbot commented Jan 19, 2024

github-actions bot commented Jan 19, 2024 •

edited

Loading

jdoerfert left a comment

arsenm left a comment

vedantparanjape-amd commented May 31, 2024 •

edited

Loading

arsenm commented May 31, 2024

vedantparanjape-amd commented May 31, 2024 •

edited

Loading

arsenm commented May 31, 2024

vedantparanjape-amd commented May 31, 2024

vedantparanjape-amd commented May 31, 2024

arsenm commented May 31, 2024

vedantparanjape-amd commented May 31, 2024

vedantparanjape-amd commented Jun 3, 2024

nikic Jul 12, 2024

vedantparanjape-amd Jul 12, 2024 •

edited

Loading

arsenm Jul 12, 2024

vedantparanjape-amd Jul 12, 2024 •

edited

Loading

vedantparanjape-amd Jul 12, 2024

[ArgPromotion] Handle pointer arguments of recursive calls #78735

[ArgPromotion] Handle pointer arguments of recursive calls #78735

Conversation

vedantparanjape-amd commented Jan 19, 2024 • edited Loading

llvmbot commented Jan 19, 2024

github-actions bot commented Jan 19, 2024 • edited Loading

jdoerfert left a comment

Choose a reason for hiding this comment

arsenm left a comment

Choose a reason for hiding this comment

vedantparanjape-amd commented May 31, 2024 • edited Loading

arsenm commented May 31, 2024

vedantparanjape-amd commented May 31, 2024 • edited Loading

arsenm commented May 31, 2024

vedantparanjape-amd commented May 31, 2024

vedantparanjape-amd commented May 31, 2024

arsenm commented May 31, 2024

vedantparanjape-amd commented May 31, 2024

vedantparanjape-amd commented Jun 3, 2024

nikic Jul 12, 2024

Choose a reason for hiding this comment

vedantparanjape-amd Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

arsenm Jul 12, 2024

Choose a reason for hiding this comment

vedantparanjape-amd Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

vedantparanjape-amd Jul 12, 2024

Choose a reason for hiding this comment

vedantparanjape-amd commented Jan 19, 2024 •

edited

Loading

github-actions bot commented Jan 19, 2024 •

edited

Loading

vedantparanjape-amd commented May 31, 2024 •

edited

Loading

vedantparanjape-amd commented May 31, 2024 •

edited

Loading

vedantparanjape-amd Jul 12, 2024 •

edited

Loading

vedantparanjape-amd Jul 12, 2024 •

edited

Loading