[CodeGen] Allow `CodeGenPassBuilder` to add module pass after function pass #77084

paperchalice · 2024-01-05T11:52:58Z

In fact, there are several backends, e.g. AArch64, AMDGPU etc. add module pass after function pass, this patch removes this constraint. This patch also adds a simple unit test for CodeGenPassBuilder.

arsenm · 2024-01-10T14:05:26Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

+      if constexpr (is_detected<is_module_pass_t, PassT>::value &&
+                    !is_detected<is_function_pass_t, PassT>::value) {


Why are both conditions necessary? Can there really be a pass that is both a module and function pass?

It seems ok to define two run methods with different ir unit in one pass, pass manager will pick the right one. Here is redundant, will remove it

arsenm

lgtm with nits

arsenm · 2024-01-11T03:54:22Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

-    addPass(createFunctionToLoopPassAdaptor(
-        LoopStrengthReducePass(), /*UseMemorySSA*/ true, Opt.DebugPM));
+    addPass(createFunctionToLoopPassAdaptor(LoopStrengthReducePass(),
+                                            /*UseMemorySSA*/ true));


Something weird happened here with whitespace? Is GitHub showing tabs?

At least in my local editor, it is whitespace.

arsenm · 2024-01-11T03:55:28Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+        "test",
+        "-print-pipeline-passes",
+    };
+    int argc = sizeof(argv) / sizeof(char *);


this can use std::size

arsenm · 2024-01-11T03:57:22Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+      "FinalizeISelPass,EarlyTailDuplicatePass,OptimizePHIsPass,"
+      "StackColoringPass,LocalStackSlotPass,DeadMachineInstructionElimPass,"
+      "EarlyMachineLICMPass,MachineCSEPass,MachineSinkingPass,"
+      "PeepholeOptimizerPass,DeadMachineInstructionElimPass,"


At some point we should probably do something about codegen passes not using the same naming convention as IR passes

Yes, currently only PassT::name() is available.

arsenm · 2024-01-11T03:57:27Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+    auto PassName = PIC.getPassNameForClassName(Name);
+    return PassName.empty() ? Name : PassName;
+  });
+  char ExpectedMIRPipeline[] =


arsenm · 2024-01-11T03:57:32Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+    auto PassName = PIC.getPassNameForClassName(Name);
+    return PassName.empty() ? Name : PassName;
+  });
+  char ExpectedIRPipeline[] =


paperchalice · 2024-01-11T04:18:41Z

Address the comments above.

arsenm · 2024-01-11T05:02:09Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

-    addPass(createFunctionToLoopPassAdaptor(
-        LoopStrengthReducePass(), /*UseMemorySSA*/ true, Opt.DebugPM));
+    addPass(createFunctionToLoopPassAdaptor(LoopStrengthReducePass(),
+                                            /*UseMemorySSA*/ true));


UseMemorySSA= so clang-format recognizes it

aeubanks

makes sense, some comments

aeubanks · 2024-01-11T05:17:05Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

+                    "Only module pass and function pass are supported.");
+
+      // Add Function Pass
+      if constexpr (is_detected<is_function_pass_t, PassT>::value)


for consistency, add braces here when else block uses braces

aeubanks · 2024-01-11T05:26:59Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+    return PassName.empty() ? Name : PassName;
+  });
+  const char ExpectedMIRPipeline[] =
+      "FinalizeISelPass,EarlyTailDuplicatePass,OptimizePHIsPass,"


imo checking the exact pipeline should be a normal lit tests, having it as a unit test is annoying to update since it takes so long to compile unittests.

for now since we don't have the infra to do this from llc this is ok, but add a TODO to move this to a normal lit test once we can do that

to prevent having to update this as we keep making changes in this space, I'd just check that no-op-module,function(no-op-function,no-op-function,no-op-function),no-op-module is a substring

aeubanks · 2024-01-11T05:28:16Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+  }
+
+  void SetUp() override {
+    std::string TripleName = Triple::normalize(sys::getDefaultTargetTriple());


isn't the pipeline going to be inconsistent depending on the default target triple?

Yes, it will depend on the specific target, but I noticed there is a BogusTargetMachine in unittests/CodeGen/MFCommon.inc, will use it later.

Uh,CodeGenPassBuilder needs MCAsmInfo but BogusTargetMachine doesn't provide it, but now we can ignore it and use default target machine because currently the pipeline provider is not the target machine.

Would be slightly safer to just pick a target machine and go with it, and disable the test if the target's not built

Would be slightly safer to just pick a target machine and go with it, and disable the test if the target's not built

Pick the default target machine and skip test when default target is not built now.

I think arsenm meant choose a specific target like x86_64-linux-gnu and bail if that target isn't available (I think you did this in a different patch's test)

aeubanks · 2024-01-11T05:29:25Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+
+    static const char *argv[] = {
+        "test",
+        "-print-pipeline-passes",


oh man this is a huge hack, but I guess this goes along with the TODO I suggested below

aeubanks · 2024-01-11T05:34:53Z

llvm/unittests/CodeGen/CodeGenPassBuilderTest.cpp

+      "lower-constant-intrinsics,UnreachableBlockElimPass,consthoist,"
+      "ReplaceWithVeclib,partially-inline-libcalls,ee-instrument<post-inline>,"
+      "scalarize-masked-mem-intrin,ExpandReductionsPass,codegenprepare,"
+      "dwarf-eh-prepare),no-op-module,function(no-op-function,no-op-function,"


no-op-function/module are getting recognized just because we happen to have the same name for the NoOpFunction/ModulePass in PassBuilder.cpp, that's not very evident or nice. let's just move those in PassBuilder.cpp to the header PassBuilder.h so unittests can use them

…n pass In fact, there are several backends, e.g. AArch64, AMDGPU etc. add module pass after function pass, this patch removes this constraint. This patch also adds a simple unit test for `CodeGenPassBuilder`

paperchalice · 2024-01-11T06:34:11Z

Address comments above.

paperchalice · 2024-01-11T11:50:21Z

Smoke test looks good, will land this if no further comments.

aeubanks

lg after the one test comment is addressed

nico · 2024-01-12T01:15:30Z

Looks like this might be breaking tests on Mac: http://45.33.8.238/macm1/76477/step_11.txt

Please take a look. Does the failure make sense to you?

paperchalice · 2024-01-12T01:22:40Z

Looks like this might be breaking tests on Mac: http://45.33.8.238/macm1/76477/step_11.txt

Please take a look. Does the failure make sense to you?

I noticed that, fix pr is opened #77860.

paperchalice · 2024-01-12T01:30:44Z

If build bots still report error, feel free to revert it.

…n pass (llvm#77084) In fact, there are several backends, e.g. AArch64, AMDGPU etc. add module pass after function pass, this patch removes this constraint. This patch also adds a simple unit test for `CodeGenPassBuilder`.

paperchalice requested a review from arsenm January 5, 2024 11:52

paperchalice force-pushed the cgpassbuildertest branch from d62a7bf to 9755fa6 Compare January 6, 2024 01:29

paperchalice mentioned this pull request Jan 7, 2024

[CodeGen] Support start/stop in CodeGenPassBuilder #70912

Merged

paperchalice requested a review from aeubanks January 8, 2024 00:44

paperchalice force-pushed the cgpassbuildertest branch 3 times, most recently from b66e697 to f5e6c04 Compare January 10, 2024 13:01

arsenm reviewed Jan 10, 2024

View reviewed changes

paperchalice force-pushed the cgpassbuildertest branch from f5e6c04 to 7ffc23e Compare January 10, 2024 14:35

paperchalice requested a review from arsenm January 10, 2024 14:35

paperchalice mentioned this pull request Jan 11, 2024

[Pass] Add some missing passes #77600

Closed

paperchalice force-pushed the cgpassbuildertest branch from 7ffc23e to 1d84f8f Compare January 11, 2024 00:46

arsenm approved these changes Jan 11, 2024

View reviewed changes

paperchalice force-pushed the cgpassbuildertest branch from 1d84f8f to 838bcce Compare January 11, 2024 04:18

arsenm approved these changes Jan 11, 2024

View reviewed changes

aeubanks reviewed Jan 11, 2024

View reviewed changes

paperchalice added 3 commits January 11, 2024 13:35

[CodeGen] Allow CodeGenPassBuilder to add module pass after functio…

b26b0a5

…n pass In fact, there are several backends, e.g. AArch64, AMDGPU etc. add module pass after function pass, this patch removes this constraint. This patch also adds a simple unit test for `CodeGenPassBuilder`

[CodeGen] Remove obsolete DebugPM flag in CodeGenPassBuilder.h

9f52e36

[Pass] Move no-op passes declaration to PassBuilder.h

a397bcc

paperchalice force-pushed the cgpassbuildertest branch from 838bcce to 0237049 Compare January 11, 2024 06:34

Address comments

251e9bf

paperchalice force-pushed the cgpassbuildertest branch from 0237049 to 251e9bf Compare January 11, 2024 06:54

paperchalice mentioned this pull request Jan 11, 2024

[NewPM][CodeGen] add TargetPassConfig like API #70906

Open

paperchalice requested review from aeubanks and arsenm January 11, 2024 12:16

pravinjagtap self-requested a review January 11, 2024 12:52

aeubanks approved these changes Jan 11, 2024

View reviewed changes

paperchalice merged commit ae1c1ed into llvm:main Jan 12, 2024
4 checks passed

paperchalice deleted the cgpassbuildertest branch January 20, 2024 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CodeGen] Allow `CodeGenPassBuilder` to add module pass after function pass #77084

[CodeGen] Allow `CodeGenPassBuilder` to add module pass after function pass #77084

paperchalice commented Jan 5, 2024

arsenm Jan 10, 2024

paperchalice Jan 10, 2024 •

edited

paperchalice Jan 10, 2024

arsenm left a comment

arsenm Jan 11, 2024

paperchalice Jan 11, 2024

arsenm Jan 11, 2024

arsenm Jan 11, 2024

paperchalice Jan 11, 2024

arsenm Jan 11, 2024

arsenm Jan 11, 2024

paperchalice commented Jan 11, 2024

arsenm Jan 11, 2024

aeubanks left a comment

aeubanks Jan 11, 2024

aeubanks Jan 11, 2024

aeubanks Jan 11, 2024

aeubanks Jan 11, 2024

paperchalice Jan 11, 2024

paperchalice Jan 11, 2024

arsenm Jan 11, 2024

paperchalice Jan 11, 2024

aeubanks Jan 11, 2024

aeubanks Jan 11, 2024

aeubanks Jan 11, 2024

paperchalice commented Jan 11, 2024

paperchalice commented Jan 11, 2024

aeubanks left a comment

nico commented Jan 12, 2024

paperchalice commented Jan 12, 2024 •

edited

paperchalice commented Jan 12, 2024

		if constexpr (is_detected<is_module_pass_t, PassT>::value &&
		!is_detected<is_function_pass_t, PassT>::value) {

[CodeGen] Allow CodeGenPassBuilder to add module pass after function pass #77084

[CodeGen] Allow CodeGenPassBuilder to add module pass after function pass #77084

Conversation

paperchalice commented Jan 5, 2024

Choose a reason for hiding this comment

paperchalice Jan 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsenm 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

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 commented Jan 11, 2024

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

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

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 commented Jan 11, 2024

paperchalice commented Jan 11, 2024

aeubanks left a comment

Choose a reason for hiding this comment

nico commented Jan 12, 2024

paperchalice commented Jan 12, 2024 • edited

paperchalice commented Jan 12, 2024

[CodeGen] Allow `CodeGenPassBuilder` to add module pass after function pass #77084

[CodeGen] Allow `CodeGenPassBuilder` to add module pass after function pass #77084

paperchalice Jan 10, 2024 •

edited

paperchalice commented Jan 12, 2024 •

edited