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

[MachineSink][AArch64] Enable sink-and-fold by default #72132

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

momchil-velikov
Copy link
Collaborator

Enable the optimisation by default for AArch64 after a compile time regressoin fix in e8209b2

Enable the optimisation by default for AArch64 after a
compile time regressoin fix in e8209b2
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 13, 2023

@llvm/pr-subscribers-backend-aarch64

Author: Momchil Velikov (momchil-velikov)

Changes

Enable the optimisation by default for AArch64 after a compile time regressoin fix in e8209b2


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

1 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64TargetMachine.cpp (+1-1)
diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
index 3d818c76bd4b7d7..fcc30a7cfceaf47 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -200,7 +200,7 @@ static cl::opt<bool> EnableGISelLoadStoreOptPostLegal(
 static cl::opt<bool>
     EnableSinkFold("aarch64-enable-sink-fold",
                    cl::desc("Enable sinking and folding of instruction copies"),
-                   cl::init(false), cl::Hidden);
+                   cl::init(true), cl::Hidden);
 
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAArch64Target() {
   // Register the target.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the last debug info being fixed, this still LGTM. Thanks for pushing this through.

(You can probably remove the command line from llvm/test/CodeGen/AArch64/sink-and-fold.ll again).

@momchil-velikov momchil-velikov merged commit 13fe038 into llvm:main Nov 16, 2023
3 checks passed
@antmox
Copy link
Contributor

antmox commented Nov 16, 2023

Hello! It looks like this patch broke lldb-aarch64-ubuntu : https://lab.llvm.org/buildbot/#/builders/96/builds/48609
Could you please look at this ?

sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
Enable the optimisation by default for AArch64 after a compile time
regressoin fix in e8209b2
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Enable the optimisation by default for AArch64 after a compile time
regressoin fix in e8209b2
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
momchil-velikov added a commit that referenced this pull request Nov 27, 2023
…2132)"

This re-commits 13fe038 after fixing a couple of issues in the LLDB
testsuite in ef9bcac and 6b87d84
@TNorthover
Copy link
Contributor

Another potential problem, I'm afraid. Seen in https://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O3, but I think this reduced case still shows it:

define i32 @unaspack212(ptr %image, i32 %ep) {
entry:
  %add149 = add i32 %ep, 1
  %idxprom150 = zext i32 %add149 to i64
  %arrayidx151 = getelementptr i8, ptr %image, i64 %idxprom150
  br label %land.lhs.true55

land.lhs.true55:                                  ; preds = %if.then155, %land.lhs.true55, %entry
  %0 = load i8, ptr %arrayidx151, align 1
  %cmp153 = icmp eq i8 %0, 0
  br i1 %cmp153, label %if.then155, label %land.lhs.true55

if.then155:                                       ; preds = %land.lhs.true55
  store i32 0, ptr %image, align 1
  br label %land.lhs.true55
}

compiled with llc -verify-machineinstrs. Basically the sinking leaves a dangling ORRWrs that was part of the zext and marks the register we care about killed.

@momchil-velikov
Copy link
Collaborator Author

Looking at it...

@momchil-velikov
Copy link
Collaborator Author

Addressed in #75072

@momchil-velikov momchil-velikov deleted the enable-sink-and-fold branch January 30, 2024 10:35
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.

None yet

5 participants