[SelectionDAG] Disable FastISel for swiftasync functions #70741

felipepiovezan · 2023-10-30T22:37:40Z

Most (x86) swiftasync functions tend to use both SelectionDAGISel and FastISel lowering:

FastISel argument lowering can only handle C calling convention.
FastISel fails mid-BB in a number of ways, including in simple ret void instructions under certain circumstances.

This dance of SelectionDAG (argument) -> FastISel (some instructions) -> SelectionDAG(remaining instructions) is lossy; in particular, Argument information lowering is cleared after that first SelectionDAG run.

Since swiftasync functions rely heavily on proper Argument lowering for debug information, this patch disables the use of FastISel in such functions.

This was tested by compiling a big translation unit from the Swift concurrency library, and there was no measurable performance impact:

// Without patch (i.e. using FastISel)
  Time (mean ± σ):      2.416 s ±  0.016 s    [User: 2.321 s, System: 0.068 s]
  Range (min … max):    2.403 s …  2.458 s    10 runs

// With patch (i.e. not using FastISel)
  Time (mean ± σ):      2.407 s ±  0.011 s    [User: 2.313 s, System: 0.067 s]
  Range (min … max):    2.396 s …  2.424 s    10 runs

Most (x86) swiftasync functions tend to use both SelectionDAGISel and FastISel lowering: * FastISel argument lowering can only handle C calling convention. * FastISel fails mid-BB in a number of ways, including in simple `ret void` instructions under certain circumstances. This dance of SelectionDAG (argument) -> FastISel (some instructions) -> SelectionDAG(remaining instructions) is lossy; in particular, Argument information lowering is cleared after that first SelectionDAG run. Since swiftasync functions rely heavily on proper Argument lowering for debug information, this patch disables the use of FastISel in such functions. This was tested by compiling a big translation unit from the Swift concurrency library, and there was no measurable performance impact: / Without patch (i.e. using FastISel) Time (mean ± σ): 2.416 s ± 0.016 s [User: 2.321 s, System: 0.068 s] Range (min … max): 2.403 s … 2.458 s 10 runs // With patch (i.e. not using FastISel) Time (mean ± σ): 2.407 s ± 0.011 s [User: 2.313 s, System: 0.067 s] Range (min … max): 2.396 s … 2.424 s 10 runs

llvmbot · 2023-10-30T22:38:44Z

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-selectiondag

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

Most (x86) swiftasync functions tend to use both SelectionDAGISel and FastISel lowering:

FastISel argument lowering can only handle C calling convention.
FastISel fails mid-BB in a number of ways, including in simple ret void instructions under certain circumstances.

This dance of SelectionDAG (argument) -> FastISel (some instructions) -> SelectionDAG(remaining instructions) is lossy; in particular, Argument information lowering is cleared after that first SelectionDAG run.

Since swiftasync functions rely heavily on proper Argument lowering for debug information, this patch disables the use of FastISel in such functions.

This was tested by compiling a big translation unit from the Swift concurrency library, and there was no measurable performance impact:

// Without patch (i.e. using FastISel)
  Time (mean ± σ):      2.416 s ±  0.016 s    [User: 2.321 s, System: 0.068 s]
  Range (min … max):    2.403 s …  2.458 s    10 runs

// With patch (i.e. not using FastISel)
  Time (mean ± σ):      2.407 s ±  0.011 s    [User: 2.313 s, System: 0.067 s]
  Range (min … max):    2.396 s …  2.424 s    10 runs

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

1 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+11-1)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 5be9ff0300b0485..d9d1b7d21a3c528 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -1441,11 +1441,21 @@ static void processSingleLocVars(FunctionLoweringInfo &FuncInfo,
   }
 }
 
+static bool shouldEnableFastISel(const Function &Fn) {
+  // Don't enable FastISel for functions with swiftasync Arguments.
+  // Debug info on those is reliant on good Argument lowering, and FastISel is
+  // not capable of lowering the entire function. Mixing the two selectors tend
+  // to result in poor lowering of Arguments.
+  return none_of(Fn.args(), [](const Argument &Arg) {
+    return Arg.hasAttribute(Attribute::AttrKind::SwiftAsync);
+  });
+}
+
 void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
   FastISelFailed = false;
   // Initialize the Fast-ISel state, if needed.
   FastISel *FastIS = nullptr;
-  if (TM.Options.EnableFastISel) {
+  if (TM.Options.EnableFastISel && shouldEnableFastISel(Fn)) {
     LLVM_DEBUG(dbgs() << "Enabling fast-isel\n");
     FastIS = TLI->createFastISel(*FuncInfo, LibInfo);
   }

arsenm · 2023-10-31T02:02:39Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

@@ -1441,11 +1441,21 @@ static void processSingleLocVars(FunctionLoweringInfo &FuncInfo,
  }
 }

+static bool shouldEnableFastISel(const Function &Fn) {


I thought we had some kind of hook for this somewhere already; I would think this is a target specific decision

OptLevelChanger ?

Thanks for the pointers, I will have a look! It would be nice if this could be done on a per function basis though, as we wouldn't want to disable fast isel for the entire module

AFAICT all the TargetMachine hooks operate independently of the Function being lowered.

Regarding OptLevelChanger, it seems to be used to force the opt level to "None" when we have skipPass(Fn) == true, and then it queries the TM to check if FastISel is enabled for O0.

That said, some of the debug messages inside OptLevelChanger makes me think we could add move the check from this patch in there. What do you think?

It's worth a try, it's supposed to allow opt level override at the function level, which is precisely what we're after.

@RKSimon @arsenm could you check if this new approach is what you had in mind?

adrian-prantl

Given that this only affects -Onone, and only x86_64, I'm fine with this.

adrian-prantl · 2023-10-31T23:32:19Z

(assuming all other comments are being addressed)

RKSimon · 2023-11-01T10:53:56Z

Do you have any tests that you can add? Particularly the improved Argument lowering handling?

felipepiovezan · 2023-11-02T14:59:24Z

Do you have any tests that you can add? Particularly the improved Argument lowering handling?

Yup, good thing you asked, because I've just found a bug with my OptLevelChanger version (the "if the optimization level changed" bit is problematic")

felipepiovezan · 2023-11-02T18:18:52Z

Added a test and tweaked the implementation slightly

felipepiovezan · 2023-11-09T17:23:52Z

Any other comments / concerns with the implementation?

RKSimon

LGTM

Most (x86) swiftasync functions tend to use both SelectionDAGISel and FastISel lowering: * FastISel argument lowering can only handle C calling convention. * FastISel fails mid-BB in a number of ways, including in simple `ret void` instructions under certain circumstances. This dance of SelectionDAG (argument) -> FastISel (some instructions) -> SelectionDAG(remaining instructions) is lossy; in particular, Argument information lowering is cleared after that first SelectionDAG run. Since swiftasync functions rely heavily on proper Argument lowering for debug information, this patch disables the use of FastISel in such functions. (cherry picked from commit 83729e6)

…le_fast_isel2 [cherry-pick][SelectionDAG] Disable FastISel for swiftasync functions (llvm#70741)

Most (x86) swiftasync functions tend to use both SelectionDAGISel and FastISel lowering: * FastISel argument lowering can only handle C calling convention. * FastISel fails mid-BB in a number of ways, including in simple `ret void` instructions under certain circumstances. This dance of SelectionDAG (argument) -> FastISel (some instructions) -> SelectionDAG(remaining instructions) is lossy; in particular, Argument information lowering is cleared after that first SelectionDAG run. Since swiftasync functions rely heavily on proper Argument lowering for debug information, this patch disables the use of FastISel in such functions.

felipepiovezan requested a review from adrian-prantl October 30, 2023 22:38

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Oct 30, 2023

felipepiovezan added the debuginfo label Oct 30, 2023

arsenm reviewed Oct 31, 2023

View reviewed changes

fixup! [SelectionDAG] Disable FastISel for swiftasync functions

91addc5

adrian-prantl approved these changes Oct 31, 2023

View reviewed changes

fixup! fixup! [SelectionDAG] Disable FastISel for swiftasync functions

9e2a70e

llvmbot added the backend:X86 label Nov 2, 2023

RKSimon approved these changes Nov 13, 2023

View reviewed changes

felipepiovezan merged commit 83729e6 into llvm:main Nov 13, 2023

felipepiovezan deleted the felipe/disable_fastisel_upstream branch November 13, 2023 16:27

felipepiovezan added a commit to swiftlang/llvm-project that referenced this pull request Nov 14, 2023

Merge pull request #7787 from felipepiovezan/felipe/cherry_pick_disab…

21398db

…le_fast_isel2 [cherry-pick][SelectionDAG] Disable FastISel for swiftasync functions (llvm#70741)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SelectionDAG] Disable FastISel for swiftasync functions #70741

[SelectionDAG] Disable FastISel for swiftasync functions #70741

Uh oh!

felipepiovezan commented Oct 30, 2023

Uh oh!

llvmbot commented Oct 30, 2023 •

edited

Loading

Uh oh!

arsenm Oct 31, 2023

Uh oh!

RKSimon Oct 31, 2023

Uh oh!

felipepiovezan Oct 31, 2023 •

edited

Loading

Uh oh!

felipepiovezan Oct 31, 2023

Uh oh!

RKSimon Oct 31, 2023

Uh oh!

felipepiovezan Oct 31, 2023

Uh oh!

adrian-prantl left a comment

Uh oh!

adrian-prantl commented Oct 31, 2023

Uh oh!

RKSimon commented Nov 1, 2023

Uh oh!

felipepiovezan commented Nov 2, 2023

Uh oh!

felipepiovezan commented Nov 2, 2023

Uh oh!

felipepiovezan commented Nov 9, 2023

Uh oh!

RKSimon left a comment

Uh oh!

Uh oh!

[SelectionDAG] Disable FastISel for swiftasync functions #70741

[SelectionDAG] Disable FastISel for swiftasync functions #70741

Uh oh!

Conversation

felipepiovezan commented Oct 30, 2023

Uh oh!

llvmbot commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

RKSimon Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

RKSimon Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented Oct 31, 2023

Uh oh!

RKSimon commented Nov 1, 2023

Uh oh!

felipepiovezan commented Nov 2, 2023

Uh oh!

felipepiovezan commented Nov 2, 2023

Uh oh!

felipepiovezan commented Nov 9, 2023

Uh oh!

RKSimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Oct 30, 2023 •

edited

Loading

felipepiovezan Oct 31, 2023 •

edited

Loading