Skip to content
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

[CloneFunction] Remove check that is no longer necessary #92577

Merged
merged 1 commit into from
May 20, 2024

Conversation

danilaml
Copy link
Collaborator

We do not need to concern ourselves with CGSCC since all remaining CG related updates went away in fa6ea7a as pointed out by @nikic in #87963 (comment).

We do not need to concern ourselves with CGSCC since all remaining
CG related updates went away in fa6ea7a.
@llvmbot
Copy link

llvmbot commented May 17, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Danila Malyutin (danilaml)

Changes

We do not need to concern ourselves with CGSCC since all remaining CG related updates went away in fa6ea7a as pointed out by @nikic in #87963 (comment).


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/CloneFunction.cpp (-7)
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 981183682b8bf..1fef8bc461211 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -825,13 +825,6 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
       if (!NewI)
         continue;
 
-      // Skip over non-intrinsic callsites, we don't want to remove any nodes
-      // from the CGSCC.
-      CallBase *CB = dyn_cast<CallBase>(NewI);
-      if (CB && CB->getCalledFunction() &&
-          !CB->getCalledFunction()->isIntrinsic())
-        continue;
-
       if (Value *V = simplifyInstruction(NewI, DL)) {
         NewI->replaceAllUsesWith(V);
 

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.

Could you please add a test for the additional folding this enables?

@danilaml
Copy link
Collaborator Author

@nikic have some downstream function calls that can be simplified but not constant folded. Will see if I can come up with a test for current main.

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.

It looks like InstSimplify only does constant folding for non-intrinsics, so probably the only way to hit this with upstream LLVM is if it's constant folding that involves a loop phi in some way and thus only happens late?

Given that, constructing a test case may be tricky, so I'm fine with landing it as-is.

@danilaml
Copy link
Collaborator Author

@nikic Yeah, wasn't able to come up with the test. Another thing is that upstream simplifyCall currently only handles poison/undef callee (wasn't skipped due to CB->getCalledFunction() check), intrinsics (wasn't skipped by the check) and also attempt to constant fold call (was already done before), but I still think it's good to remove this "dead" code with outdated comment.

@danilaml danilaml merged commit 36899d6 into llvm:main May 20, 2024
6 checks passed
@danilaml danilaml deleted the remove-dead-code branch May 20, 2024 13:51
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.

3 participants