[AArch64] Add AArch64PassRegistry.def #85215

paperchalice · 2024-03-14T12:38:44Z

PR #83567 ports SelectionDAGISel to the new pass manager, then each backend should provide <Target>DagToDagISel() in new pass manager style. Then each target should provide <Target>PassRegistry.def to register backend passes in registerPassBuilderCallbacks to reduce duplicate code.
This PR adds AArch64PassRegistry.def to AArch64 backend and boilerplate code in registerPassBuilderCallbacks.

llvmbot · 2024-03-14T12:39:15Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-transforms

Author: None (paperchalice)

Changes

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

5 Files Affected:

(added) llvm/lib/Target/AArch64/AArch64CodeGenPassBuilder.cpp (+40)
(added) llvm/lib/Target/AArch64/AArch64PassRegistry.def (+19)
(modified) llvm/lib/Target/AArch64/AArch64TargetMachine.cpp (-9)
(modified) llvm/lib/Target/AArch64/CMakeLists.txt (+1)
(modified) llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll (+3)

diff --git a/llvm/lib/Target/AArch64/AArch64CodeGenPassBuilder.cpp b/llvm/lib/Target/AArch64/AArch64CodeGenPassBuilder.cpp
new file mode 100644
index 00000000000000..791cb3728c1ce3
--- /dev/null
+++ b/llvm/lib/Target/AArch64/AArch64CodeGenPassBuilder.cpp
@@ -0,0 +1,40 @@
+//===-- AArch64CodeGenPassBuilder.cpp -----------------------------*- C++ -*-=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+/// \file
+/// This file contains AArch64 CodeGen pipeline builder.
+/// TODO: Port CodeGen passes to new pass manager.
+//===----------------------------------------------------------------------===//
+
+#include "AArch64LoopIdiomTransform.h"
+#include "AArch64TargetMachine.h"
+
+void AArch64TargetMachine::registerPassBuilderCallbacks(
+    PassBuilder &PB, bool PopulateClassToPassNames) {
+  if (PopulateClassToPassNames) {
+    auto *PIC = PB.getPassInstrumentationCallbacks();
+#define LOOP_PASS(NAME, CREATE_PASS)                                           \
+  PIC->addClassToPassName(decltype(CREATE_PASS)::name(), NAME);
+#include "AArch64PassRegistry.def"
+  }
+
+  PB.registerPipelineParsingCallback(
+      [](StringRef Name, LoopPassManager &LPM, ArrayRef<PipelineElement>) {
+#define LOOP_PASS(NAME, CREATE_PASS)                                           \
+  if (Name == NAME) {                                                          \
+    LPM.addPass(CREATE_PASS);                                                  \
+    return true;                                                               \
+  }
+#include "AArch64PassRegistry.def"
+        return false;
+      });
+
+  PB.registerLateLoopOptimizationsEPCallback(
+      [=](LoopPassManager &LPM, OptimizationLevel Level) {
+        LPM.addPass(AArch64LoopIdiomTransformPass());
+      });
+}
diff --git a/llvm/lib/Target/AArch64/AArch64PassRegistry.def b/llvm/lib/Target/AArch64/AArch64PassRegistry.def
new file mode 100644
index 00000000000000..db566d45620022
--- /dev/null
+++ b/llvm/lib/Target/AArch64/AArch64PassRegistry.def
@@ -0,0 +1,19 @@
+//===- AArch64PassRegistry.def - Registry of AArch64 passes -----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is used as the registry of passes that are part of the X86 backend.
+//
+//===----------------------------------------------------------------------===//
+
+// NOTE: NO INCLUDE GUARD DESIRED!
+
+#ifndef LOOP_PASS
+#define LOOP_PASS(NAME, CREATE_PASS)
+#endif
+LOOP_PASS("aarch64-lit", AArch64LoopIdiomTransformPass())
+#undef LOOP_PASS
diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
index e5e60459e8148a..f4bcf6d68b52c3 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -11,7 +11,6 @@
 
 #include "AArch64TargetMachine.h"
 #include "AArch64.h"
-#include "AArch64LoopIdiomTransform.h"
 #include "AArch64MachineFunctionInfo.h"
 #include "AArch64MachineScheduler.h"
 #include "AArch64MacroFusion.h"
@@ -545,14 +544,6 @@ class AArch64PassConfig : public TargetPassConfig {
 
 } // end anonymous namespace
 
-void AArch64TargetMachine::registerPassBuilderCallbacks(
-    PassBuilder &PB, bool PopulateClassToPassNames) {
-  PB.registerLateLoopOptimizationsEPCallback(
-      [=](LoopPassManager &LPM, OptimizationLevel Level) {
-        LPM.addPass(AArch64LoopIdiomTransformPass());
-      });
-}
-
 TargetTransformInfo
 AArch64TargetMachine::getTargetTransformInfo(const Function &F) const {
   return TargetTransformInfo(AArch64TTIImpl(this, F));
diff --git a/llvm/lib/Target/AArch64/CMakeLists.txt b/llvm/lib/Target/AArch64/CMakeLists.txt
index 95b228f293204e..7aa29fce60ab1b 100644
--- a/llvm/lib/Target/AArch64/CMakeLists.txt
+++ b/llvm/lib/Target/AArch64/CMakeLists.txt
@@ -47,6 +47,7 @@ add_llvm_target(AArch64CodeGen
   AArch64BranchTargets.cpp
   AArch64CallingConvention.cpp
   AArch64CleanupLocalDynamicTLSPass.cpp
+  AArch64CodeGenPassBuilder.cpp
   AArch64CollectLOH.cpp
   AArch64CondBrTuning.cpp
   AArch64ConditionalCompares.cpp
diff --git a/llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll b/llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll
index e6a0c5f45375fd..e6d88de6314d29 100644
--- a/llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll
+++ b/llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll
@@ -2,6 +2,9 @@
 ; RUN: opt -aarch64-lit -aarch64-lit-verify -verify-dom-info -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s
 ; RUN: opt -aarch64-lit -simplifycfg -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s --check-prefix=LOOP-DEL
 ; RUN: opt -aarch64-lit -mtriple aarch64-unknown-linux-gnu -S < %s | FileCheck %s --check-prefix=NO-TRANSFORM
+; RUN: opt -p aarch64-lit -aarch64-lit-verify -verify-dom-info -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s
+; RUN: opt -p aarch64-lit -simplifycfg -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s --check-prefix=LOOP-DEL
+; RUN: opt -p aarch64-lit -mtriple aarch64-unknown-linux-gnu -S < %s | FileCheck %s --check-prefix=NO-TRANSFORM
 
 define i32 @compare_bytes_simple(ptr %a, ptr %b, i32 %len, i32 %extra, i32 %n) {
 ; CHECK-LABEL: define i32 @compare_bytes_simple(

aeubanks · 2024-03-14T21:22:53Z

llvm/lib/Target/AArch64/AArch64CodeGenPassBuilder.cpp

+
+using namespace llvm;
+
+void AArch64TargetMachine::registerPassBuilderCallbacks(


why was this moved to a different file? it's still a method in AArch64TargetMachine

If we choose to create a single header per pass, imo use a dedicated file to process them would be better.

I sort of see what you mean, but I think it's very confusing that one method in a class is defined in a different file than the other methods. I'd be fine with forwarding the actual implementation to a different helper function that's defined in a different file, but the actual AArch64TargetMachine method should stay in the original file

aeubanks · 2024-03-14T21:23:20Z

llvm/lib/Target/AArch64/AArch64CodeGenPassBuilder.cpp

+
+void AArch64TargetMachine::registerPassBuilderCallbacks(
+    PassBuilder &PB, bool PopulateClassToPassNames) {
+  if (PopulateClassToPassNames) {


do you think it's possible to generalize this even more so each backend doesn't have to copy paste this code?

What about a new tablegen backend?

oh man I really don't want to use tablegen for this.

I think a solution would require the preprocessor since the existing code does a lot with the preprocessor

paperchalice · 2024-03-16T04:23:49Z

New PM test is regenerated due to the LoopSimplifyPass in adaptor.

davemgreen

Can the tests re-use the same CHECK lines, and if not why are they different? Thanks

paperchalice · 2024-03-18T00:43:20Z

Can the tests re-use the same CHECK lines, and if not why are they different? Thanks

There is a loop canonicalization stage before running loop passes, it may modify CFG.

llvm-project/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

Lines 438 to 448 in c0d03d2

    
           explicit FunctionToLoopPassAdaptor(std::unique_ptr<PassConceptT> Pass, 
        
                                              bool UseMemorySSA = false, 
        
                                              bool UseBlockFrequencyInfo = false, 
        
                                              bool UseBranchProbabilityInfo = false, 
        
                                              bool LoopNestMode = false) 
        
               : Pass(std::move(Pass)), UseMemorySSA(UseMemorySSA), 
        
                 UseBlockFrequencyInfo(UseBlockFrequencyInfo), 
        
                 UseBranchProbabilityInfo(UseBranchProbabilityInfo), 
        
                 LoopNestMode(LoopNestMode) { 
        
             LoopCanonicalizationFPM.addPass(LoopSimplifyPass()); 
        
             LoopCanonicalizationFPM.addPass(LCSSAPass());

paperchalice · 2024-03-18T03:57:19Z

llvm/include/llvm/Passes/TargetPassRegistry.inc

+#define ADD_CLASS_PASS_TO_PASS_NAME(NAME, CREATE_PASS)                         \
+  PIC->addClassToPassName(decltype(CREATE_PASS)::name(), NAME);
+#define ADD_CLASS_PASS_TO_PASS_NAME_WITH_PARAMS(NAME, CLASS)                   \
+  PIC->addClassToPassName(CLASS, NAME);


Could be

#define ADD_CLASS_PASS_TO_PASS_NAME_WITH_PARAMS(NAME, CREATE_PASS) \ { \ std::function C_ = CREATE_PASS; \ PIC->addClassToPassName(decltype(C_)::result_type::name(), \ NAME); \ }

So MODULE_PASS_WITH_PARAMS(NAME, CLASS, CREATE_PASS, PARSER, PARAMS) -> MODULE_PASS_WITH_PARAMS(NAME, CREATE_PASS, PARSER, PARAMS)

paperchalice · 2024-03-18T09:45:22Z

ICE occurs when msvc compiling the following code:

int main() {
    constexpr bool b = std::size({"",}) > 1;
}

Looks like it's been fixed in msvc v19.30.

github-actions · 2024-03-18T10:02:50Z

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

fhahn · 2024-03-18T12:35:33Z

Could you please add more details in the PR description?

paperchalice · 2024-03-18T12:52:07Z

Could you please add more details in the PR description?

Done.

aeubanks

this looks like the right direction to go

aeubanks · 2024-03-19T21:56:58Z

llvm/include/llvm/Passes/TargetPassRegistry.inc

+    return true;                                                               \
+  }
+
+  if constexpr (HAVE_MODULE_PASS_) {


I'd keep things simple and not have the constexpr logic (meaning always add parsing callbacks) since pipeline parsing speed is not important for compile times. Unless you have a specific reason for doing so?

Just reduce the number of callbacks, will clear it later.

aeubanks · 2024-03-20T06:40:41Z

llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll

@@ -2,6 +2,9 @@
 ; RUN: opt -aarch64-lit -aarch64-lit-verify -verify-dom-info -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s
 ; RUN: opt -aarch64-lit -simplifycfg -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s --check-prefix=LOOP-DEL
 ; RUN: opt -aarch64-lit -mtriple aarch64-unknown-linux-gnu -S < %s | FileCheck %s --check-prefix=NO-TRANSFORM
+; RUN: opt -p aarch64-lit -aarch64-lit-verify -verify-dom-info -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s --check-prefix=CHECK-NPM


do you know why the new PM pass gives different results? ideally we wouldn't be adding thousands of CHECK lines

Input pipeline isfunction(loop(aarch64-lit)), the real pipeline is loop-simplify,lcssa,loop(aarch64-lit)
The LoopSimplifiy pass modifies the cfg here.

llvm-project/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

Lines 438 to 448 in c0d03d2

explicit FunctionToLoopPassAdaptor(std::unique_ptr<PassConceptT> Pass,

bool UseMemorySSA = false,

bool UseBlockFrequencyInfo = false,

bool UseBranchProbabilityInfo = false,

bool LoopNestMode = false)

: Pass(std::move(Pass)), UseMemorySSA(UseMemorySSA),

UseBlockFrequencyInfo(UseBlockFrequencyInfo),

UseBranchProbabilityInfo(UseBranchProbabilityInfo),

LoopNestMode(LoopNestMode) {

LoopCanonicalizationFPM.addPass(LoopSimplifyPass());

LoopCanonicalizationFPM.addPass(LCSSAPass());

makes sense, but can figure out how to not add thousands of CHECK lines just to make sure the new PM pass works? either create a new test, or change the test to already be in loop simplify and LCSSA form (committed in a separate change and rebase this change on that)

aeubanks

in general looks good, but a couple of comments

aeubanks · 2024-03-20T16:13:07Z

llvm/include/llvm/Passes/TargetPassRegistry.inc

+});
+
+PB.registerParseAACallback([](StringRef Name, AAManager &AM) {
+#define FUNCTION_ANALYSIS(NAME, CREATE_PASS)                                   \


this seems wrong, we're registering an AA analysis for every function analysis?

I think we can just drop this part and let specific backends that have their own AA manually register them

Oops, forgot to update it.

NVPTX and AMDGPU meed it, currently there's no harm in keeping it 🤔.

aeubanks · 2024-03-20T16:26:40Z

llvm/test/Transforms/LoopIdiom/AArch64/byte-compare-index.ll

@@ -2,6 +2,9 @@
 ; RUN: opt -aarch64-lit -aarch64-lit-verify -verify-dom-info -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s
 ; RUN: opt -aarch64-lit -simplifycfg -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s --check-prefix=LOOP-DEL
 ; RUN: opt -aarch64-lit -mtriple aarch64-unknown-linux-gnu -S < %s | FileCheck %s --check-prefix=NO-TRANSFORM
+; RUN: opt -p aarch64-lit -aarch64-lit-verify -verify-dom-info -mtriple aarch64-unknown-linux-gnu -mattr=+sve -S < %s | FileCheck %s --check-prefix=CHECK-NPM


makes sense, but can figure out how to not add thousands of CHECK lines just to make sure the new PM pass works? either create a new test, or change the test to already be in loop simplify and LCSSA form (committed in a separate change and rebase this change on that)

Make this test case work on both new and legacy pass manager. See also llvm#85215

aeubanks · 2024-03-21T02:11:31Z

I'd put [NewPM] in the commit title

Make this test case work on both new and legacy pass manager. See also #85215

Make this test case work on both new and legacy pass manager. See also llvm#85215

PR llvm#83567 ports `SelectionDAGISel` to the new pass manager, then each backend should provide `<Target>DagToDagISel()` in new pass manager style. Then each target should provide `<Target>PassRegistry.def` to register backend passes in `registerPassBuilderCallbacks` to reduce duplicate code. This PR adds `AArch64PassRegistry.def` to AArch64 backend and boilerplate code in `registerPassBuilderCallbacks`.

llvmbot added backend:AArch64 llvm:transforms labels Mar 14, 2024

paperchalice force-pushed the aarch64-registry branch 5 times, most recently from fe025b4 to 228002a Compare March 14, 2024 14:02

paperchalice requested review from david-arm and aeubanks March 14, 2024 14:02

aeubanks reviewed Mar 14, 2024

View reviewed changes

paperchalice force-pushed the aarch64-registry branch 2 times, most recently from 7533bc6 to f887646 Compare March 16, 2024 04:22

paperchalice force-pushed the aarch64-registry branch 2 times, most recently from f5ee6a5 to 0a98ff8 Compare March 16, 2024 13:06

davemgreen reviewed Mar 17, 2024

View reviewed changes

paperchalice force-pushed the aarch64-registry branch 2 times, most recently from 3f5a2ef to bde44fb Compare March 18, 2024 03:50

paperchalice commented Mar 18, 2024

View reviewed changes

paperchalice requested a review from aeubanks March 18, 2024 08:51

paperchalice force-pushed the aarch64-registry branch from 76846e5 to eff29b2 Compare March 18, 2024 10:04

aeubanks reviewed Mar 20, 2024

View reviewed changes

paperchalice force-pushed the aarch64-registry branch from cd962e1 to c7a2423 Compare March 20, 2024 13:23

paperchalice force-pushed the aarch64-registry branch from 9a9992f to cd962e1 Compare March 20, 2024 13:28

aeubanks reviewed Mar 20, 2024

View reviewed changes

paperchalice force-pushed the aarch64-registry branch 2 times, most recently from 5d56740 to 234c81a Compare March 21, 2024 00:46

paperchalice added a commit to paperchalice/llvm-project that referenced this pull request Mar 21, 2024

[AArch64] Run LoopSimplifyPass in byte-compare-index.ll

f0018e7

Make this test case work on both new and legacy pass manager. See also llvm#85215

paperchalice mentioned this pull request Mar 21, 2024

[AArch64] Run LoopSimplifyPass in byte-compare-index.ll #86053

Merged

paperchalice force-pushed the aarch64-registry branch from 234c81a to 7bfe5b2 Compare March 21, 2024 01:31

aeubanks approved these changes Mar 21, 2024

View reviewed changes

paperchalice added a commit that referenced this pull request Mar 21, 2024

[AArch64] Run LoopSimplifyPass in byte-compare-index.ll (#86053)

44a81af

Make this test case work on both new and legacy pass manager. See also #85215

paperchalice added 2 commits March 21, 2024 10:38

[AArch64] Add AArch64PassRegistry.def

902b09c

always add callbacks

f86cc24

paperchalice force-pushed the aarch64-registry branch from 7bfe5b2 to f86cc24 Compare March 21, 2024 02:42

paperchalice merged commit 29bf32e into llvm:main Mar 21, 2024
3 of 4 checks passed

paperchalice deleted the aarch64-registry branch March 21, 2024 02:57

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024

[AArch64] Run LoopSimplifyPass in byte-compare-index.ll (llvm#86053)

bfc50fc

Make this test case work on both new and legacy pass manager. See also llvm#85215

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AArch64] Add AArch64PassRegistry.def #85215

[AArch64] Add AArch64PassRegistry.def #85215

paperchalice commented Mar 14, 2024 •

edited

llvmbot commented Mar 14, 2024 •

edited

aeubanks Mar 14, 2024

paperchalice Mar 15, 2024

aeubanks Mar 15, 2024

aeubanks Mar 14, 2024

paperchalice Mar 15, 2024 •

edited

aeubanks Mar 15, 2024

paperchalice commented Mar 16, 2024

davemgreen left a comment

paperchalice commented Mar 18, 2024 •

edited

paperchalice Mar 18, 2024 •

edited

paperchalice commented Mar 18, 2024

github-actions bot commented Mar 18, 2024 •

edited

fhahn commented Mar 18, 2024

paperchalice commented Mar 18, 2024

aeubanks left a comment

aeubanks Mar 19, 2024

paperchalice Mar 20, 2024

aeubanks Mar 20, 2024

paperchalice Mar 20, 2024 •

edited

aeubanks Mar 20, 2024

aeubanks left a comment

aeubanks Mar 20, 2024

paperchalice Mar 21, 2024

aeubanks Mar 20, 2024

aeubanks commented Mar 21, 2024


		using namespace llvm;

		void AArch64TargetMachine::registerPassBuilderCallbacks(

	explicit FunctionToLoopPassAdaptor(std::unique_ptr<PassConceptT> Pass,
	bool UseMemorySSA = false,
	bool UseBlockFrequencyInfo = false,
	bool UseBranchProbabilityInfo = false,
	bool LoopNestMode = false)
	: Pass(std::move(Pass)), UseMemorySSA(UseMemorySSA),
	UseBlockFrequencyInfo(UseBlockFrequencyInfo),
	UseBranchProbabilityInfo(UseBranchProbabilityInfo),
	LoopNestMode(LoopNestMode) {
	LoopCanonicalizationFPM.addPass(LoopSimplifyPass());
	LoopCanonicalizationFPM.addPass(LCSSAPass());

[AArch64] Add AArch64PassRegistry.def #85215

[AArch64] Add AArch64PassRegistry.def #85215

Conversation

paperchalice commented Mar 14, 2024 • edited

llvmbot commented Mar 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice commented Mar 16, 2024

davemgreen left a comment

Choose a reason for hiding this comment

paperchalice commented Mar 18, 2024 • edited

paperchalice Mar 18, 2024 • edited

Choose a reason for hiding this comment

paperchalice commented Mar 18, 2024

github-actions bot commented Mar 18, 2024 • edited

fhahn commented Mar 18, 2024

paperchalice commented Mar 18, 2024

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks commented Mar 21, 2024

paperchalice commented Mar 14, 2024 •

edited

llvmbot commented Mar 14, 2024 •

edited

paperchalice Mar 15, 2024 •

edited

paperchalice commented Mar 18, 2024 •

edited

paperchalice Mar 18, 2024 •

edited

github-actions bot commented Mar 18, 2024 •

edited

paperchalice Mar 20, 2024 •

edited