[CodeGen][NPM] Update BranchFolderLegacy make tail merge configurable via flag #135277

mikhailramalho · 2025-04-10T23:07:21Z

This patch adds support for configuring the BranchFolderPass with or without tail merging.

…option to enable tail merge Signed-off-by: Mikhail R. Gadelha <[email protected]>

Signed-off-by: Mikhail R. Gadelha <[email protected]>

mikhailramalho

@arsenm this is a tentative patch do address your comments.

I do have a couple of questions:

mikhailramalho · 2025-04-10T23:09:02Z

llvm/lib/CodeGen/BranchFolding.cpp

  // HW that requires structurized CFG.
  bool EnableTailMerge = !MF.getTarget().requiresStructuredCFG() &&
-                         PassConfig->getEnableTailMerge();
+                         PassConfig->getEnableTailMerge() &&


This global state should be moved to only change the pass parameter

Do you mean I should remove the PassConfig->getEnableTailMerge()?

Yes, it can be passed as the EnableTailMerge parameter when TargetPassConfig creates this pass instance (where currently this is done through its ID).

Hey, so I did 9b95974, but I'm getting a weird error: when the branch folding pass runs now, I get that the pre-conditions are not valid:

MachineFunctionProperties required by Control Flow Optimizer pass are not met by function RWBufferStore_Vec4_I32. Required properties: NoPHIs Current properties: IsSSA, TracksLiveness, Legalized, Selected

This seems to only happen because of this change:

// Branch folding must be run after regalloc and prolog/epilog insertion. - addPass(&BranchFolderPassID); + addPass(createBranchFolderPass(getEnableTailMerge()));

The calling addPass with the BranchFolderPassID, it doesn't seem to be added to the list of passes because char BranchFolderLegacy::ID = 0, whereas when we call createBranchFolderPass, the pass is added and fails.

Maybe I'm missing some detail here, but the pass doesn't seem to run at all before my changes.

In the failing tests, the branch folding pass is disabled here:

llvm-project/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp

Line 174 in 32c3909

disablePass(&BranchFolderPassID);

So by calling createBranchFolderPass, we don't check the disabled passed

This also seems to bypass the -disable-branch-fold.

Any suggestions on how to proceed?

I think the function properties error is from somewhere else. Most passes probably are not appropriately reporting cleared properties

What test hit the property error? I think SPIRV needs to cear NoPHIs property after its selection. It's bypassing the usual place it's cleared (though I'm not sure why it's cleared here)

All tests are passing with the latest version of this PR

Is this avoided after the new version of #136327?

This piece of code is no longer part of the PR.

mikhailramalho · 2025-04-10T23:10:57Z

llvm/lib/CodeGen/BranchFolding.cpp


-  explicit BranchFolderLegacy() : MachineFunctionPass(ID) {}
+  explicit BranchFolderLegacy(bool EnableTailMerge = true)
+      : MachineFunctionPass(ID), EnableTailMerge(EnableTailMerge) {}


That added the initial port to the new pm. This is now changing the pass arguments in the old PM, without the matching new PM change

Are you talking about the default argument? I see that BranchFolderPass already takes a bool EnableTailMerge:

llvm-project/llvm/include/llvm/CodeGen/BranchFoldingPass.h

Line 19 in b39ab7a

BranchFolderPass(bool EnableTailMerge) : EnableTailMerge(EnableTailMerge) {}

Signed-off-by: Mikhail R. Gadelha <[email protected]>

This reverts commit f2dd02c.

…nter Signed-off-by: Mikhail R. Gadelha <[email protected]>

mikhailramalho · 2025-04-17T14:15:57Z

ping

llvm/lib/CodeGen/BranchFolding.cpp

arsenm · 2025-04-18T16:41:07Z

llvm/lib/CodeGen/TargetPassConfig.cpp

+  IdentifyingPassPtr TargetID = getPassSubstitution(PassID);
+  if (!overridePass(PassID, TargetID).isValid())
+    return;


Shouldn't need to touch the override infrastructure?

Sorry, I don't get it... What do you mean?

I had to add this to check if the pass was disabled. It's a copy-and-paste from the addPass(AnalysisID PassID) method.

Yes, that's the problem. You shouldn't have to do anything like this

I've replaced this check with a isPassSubstitutedOrOverridden check, similar to how it's done for createPrologEpilogInserterPass. Does this address your comment?

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in #135277.

Signed-off-by: Mikhail R. Gadelha <[email protected]>

llvm/lib/CodeGen/BranchFolding.cpp

Signed-off-by: Mikhail R. Gadelha <[email protected]>

github-actions · 2025-04-21T13:12:21Z

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

Signed-off-by: Mikhail R. Gadelha <[email protected]>

mikhailramalho · 2025-04-22T15:23:01Z

ping

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in #135277.

mikhailramalho · 2025-04-28T18:45:35Z

ping

cdevadas · 2025-04-29T05:30:29Z

llvm/lib/CodeGen/BranchFolding.cpp

 PreservedAnalyses BranchFolderPass::run(MachineFunction &MF,
                                        MachineFunctionAnalysisManager &MFAM) {
  MFPropsModifier _(*this, MF);
-  bool EnableTailMerge =


Since you're removing it here, the NPM require the flag requiresStructuredCFG() to be incorporated here.

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Passes/CodeGenPassBuilder.h#L1235

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in llvm#135277.

mikhailramalho added 3 commits April 10, 2025 16:08

[CodeGen][NPM] Create method to add the branch folding pass with the …

1439764

…option to enable tail merge Signed-off-by: Mikhail R. Gadelha <[email protected]>

Add printPipeline

1129f40

Signed-off-by: Mikhail R. Gadelha <[email protected]>

Remove unused include

7d8d9df

Signed-off-by: Mikhail R. Gadelha <[email protected]>

mikhailramalho requested review from arsenm and optimisan April 10, 2025 23:07

mikhailramalho commented Apr 10, 2025

View reviewed changes

mikhailramalho added 7 commits April 11, 2025 12:05

Pass global flag via argument

9b95974

Signed-off-by: Mikhail R. Gadelha <[email protected]>

Removed the default arg

17fcc45

Signed-off-by: Mikhail R. Gadelha <[email protected]>

PassConfig is no longer needed

73b228a

Signed-off-by: Mikhail R. Gadelha <[email protected]>

Implemented suggested changes

f2dd02c

Signed-off-by: Mikhail R. Gadelha <[email protected]>

Default to true

fc156b5

Signed-off-by: Mikhail R. Gadelha <[email protected]>

Revert "Implemented suggested changes"

b49d6d3

This reverts commit f2dd02c.

Check if the pass was disabled when adding the pass via the pass' poi…

1293477

…nter Signed-off-by: Mikhail R. Gadelha <[email protected]>

mikhailramalho requested review from cdevadas and ellishg April 15, 2025 18:32

arsenm reviewed Apr 18, 2025

View reviewed changes

arsenm added a commit that referenced this pull request Apr 18, 2025

SPIRV: Set NoPHIs property after selection

829c364

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in #135277.

arsenm mentioned this pull request Apr 18, 2025

SPIRV: Set NoPHIs property after rewriting them #136327

Merged

Moved requiresStructuredCFG check to pass creation

756e1a5

Signed-off-by: Mikhail R. Gadelha <[email protected]>

cdevadas requested review from aeubanks and paperchalice April 21, 2025 05:39

cdevadas reviewed Apr 21, 2025

View reviewed changes

llvm/lib/CodeGen/BranchFolding.cpp Outdated Show resolved Hide resolved

mikhailramalho added 2 commits April 21, 2025 09:53

Use isPassSubstitutedOrOverridden to check if the pass was disabled

501a1cf

Signed-off-by: Mikhail R. Gadelha <[email protected]>

Removed unnecessary local variable

61251d3

Signed-off-by: Mikhail R. Gadelha <[email protected]>

clang-format

ec4571a

Signed-off-by: Mikhail R. Gadelha <[email protected]>

arsenm added a commit that referenced this pull request Apr 23, 2025

SPIRV: Set NoPHIs property after selection

5529880

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in #135277.

arsenm added a commit that referenced this pull request Apr 23, 2025

SPIRV: Set NoPHIs property after rewriting them

76f6ab8

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in #135277.

arsenm added a commit that referenced this pull request Apr 24, 2025

SPIRV: Set NoPHIs property after rewriting them (#136327)

66461db

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in #135277.

cdevadas reviewed Apr 29, 2025

View reviewed changes

IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025

SPIRV: Set NoPHIs property after rewriting them (llvm#136327)

8e952aa

There should be no PHIs after selection, as OpPhi is used instead. This hopefully avoids errors in llvm#135277.

[CodeGen][NPM] Update BranchFolderLegacy make tail merge configurable via flag #135277

Are you sure you want to change the base?

[CodeGen][NPM] Update BranchFolderLegacy make tail merge configurable via flag #135277

Uh oh!

Conversation

mikhailramalho commented Apr 10, 2025

Uh oh!

mikhailramalho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikhailramalho commented Apr 17, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikhailramalho commented Apr 22, 2025

Uh oh!

mikhailramalho commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Apr 21, 2025 •

edited

Loading