Skip compiling fusion segments when using expression evaluator by Priya2698 · Pull Request #1930 · NVIDIA/Fuser

Priya2698 · 2024-03-13T20:47:44Z

This PR resolves Issue #1812.

Removes the workarounds added to support ATen evaluation for matmuls in PR Matmul default scheduling #1775 (in predicate_elimination.cpp and validation.cpp (see comments in the PR)
FusionExecutor::fusion_ is initialized only when compilation is skipped. So only one of lowered_ or fusion_ is non-null.
FusionKernelRuntime/FusionExecutorCache/FusionExecutor::isCompiled() indicates whether compilation was attempted (either marked for EE or via nvFuser).

Priya2698 · 2024-03-14T19:41:30Z

!build

Priya2698 · 2024-03-14T22:36:33Z

!build

Priya2698 · 2024-04-12T21:44:01Z

!build

Priya2698 · 2024-04-15T22:29:25Z

!build

Priya2698 · 2024-04-19T21:38:18Z

!build

Priya2698 · 2024-04-22T20:12:49Z

!build

Priya2698 · 2024-04-22T20:42:22Z

csrc/device_lower/analysis/predicate_elimination.cpp

@@ -890,9 +890,7 @@ class PredicateChcker : public IterVisitor {
 } // namespace

 PredicateElimination::PredicateElimination(Fusion* fusion) {


This was a WAR. Removing now, since we are skipping compilation.

Priya2698 · 2024-04-22T20:42:41Z

csrc/device_lower/validation.cpp

@@ -1030,12 +1030,7 @@ void validateSizeMemoryOp(LoadStoreOp* ldst) {
 //! Validate data format and GPU arch compatibility of scheduled
 //!  mma operators on the fusion.
 void validateMma(Fusion* fusion) {


This was a WAR. Removing now, since we are skipping compilation.

csrc/executor.h

wujingyue · 2024-04-22T21:17:43Z

csrc/serde/fusion_cache.fbs


 // Each Fusion Executor maps to a lowered and compiled kernel.
 table FusionExecutor {
+  is_compilation_skipped : bool;


Why is this needed? isCompilationSkipped is a method that describes the state of the FusionExecutor. Making it a field in table FusionExecutor seems to suggest it's part of the state itself. cc @rdspring1

It is a way to delineate when we're using the Aten function. Otherwise, we will recompile the fusion to create lowered_ during deserialization.

isCompilationSkipped indicates that we will use an Aten function and the specific fusion is passed to the FusionExecutor in compileFusion. You could separate the first part into a bool field.

Sorry for the delay -- I missed the notification.

I understood how it's used here to avoid unnecessary compilation. However, can we replace that check with a check on the fusion itself instead of introducing yet another flag? If works, this can avoid an additional state that has to be consistent with some existing state of the fusion.

csrc/kernel_cache.cpp

tests/cpp/test_resize.cpp

tests/cpp/test_gpu3.cpp

jjsjann123 · 2024-04-22T21:53:59Z

csrc/kernel_cache.h

    return std::all_of(
        executors_.begin(), executors_.end(), [](const auto& executor) {
-          return executor.isCompiled();
+          return executor.isCompiled() || executor.isCompilationSkipped();


nitpick: This feels a little bit confusing to me.

i.e. FusionKernelRuntime::isCompiled means something different from FusionExecutor::isCompiled. (similarly we have FusionExecutorCache::isCompiled.

We don't have to do it in this PR, but maybe another renaming PR to clean up the isCompiled vs isCompilationSkipped in FusionExecutor.

I should rename it to FusionKernelRuntime::isCompiledOrCompilationSkipped.
This function is used to check if compilation was already attempted which is true if either the executors are compiled or compilation was skipped for them.

Any other naming suggestions for this function?

isCompilationSkipped() --- Returns True when we use the Aten function and FusionExecutor is never compiled.

isCompiled() --- Returns False when FusionExecutor has yet to be compiled.

Maybe we should rename isCompilationSkipped() to isAtenExecuted() or isEagerModeExecutor()

I agree with @rdspring1's suggestion. We can keep isCompiled to denote if compilation was attempted -- either a kernel was compiled or we are using Aten evaluation. Additionally, isCompilationSkipped is renamed to isEagerModeExecutor.

In this naming scheme, isCompiled always means whether or not we attempted to compile a fusion. FusionExecutor::isCompiled which tests for a compiled kernel will need to be renamed though -- maybe isNvfuserExecutor?

Wdyt?

When isEagerModeExecutor is false, we're using the nvfuser executor.

isCompiled implies that we have a cubin for the given fusion. There is an implicit assumption that the nvfuser executor is being used.

I'd consider renaming isCompiled to isCubinCompiled, but leaving it alone is also fine.

Okay, so the renaming is

FusionExecutor::isCompiled -> FusionExecutor::isCubinCompiled

FusionExecutor::isCompilationSkipped -> FusionExecutor::isEagerModeExecutor

FusionKernelRuntime/FusionExecutorCache::isCompiled -> no change. Returns true if either of the above are true indicating that compilation was attempted.

I'll go with this naming if it clears the confusion. @rdspring1 @jjsjann123

sounds very reasonable to me.

csrc/executor.h

… tests

…`. (#2061) Serialization assumes that `FusionExecutor` always contains a compiled kernel. This PR updates serialization to skip compilation correctly.

Co-authored-by: Jingyue Wu <wujingyue@gmail.com>

Co-authored-by: jjsjann123 <jiej@nvidia.com>

Priya2698 · 2024-04-24T23:58:42Z

!build

csrc/executor.h

jjsjann123 · 2024-04-25T18:13:53Z

csrc/executor.h

  }

+  Fusion* fusion() const {
+    NVF_ERROR((lowered_ && !fusion_) || (!lowered_ && fusion_));


wasn't this just lowered_ ^ fusion_? An error message would be nice.

Yes, but XOR does not work on these objects.

TIL. explicit operator bool()

jjsjann123

Thanks for addressing my concerns.

Priya2698 · 2024-04-25T22:05:09Z

!build

wujingyue · 2024-04-26T18:07:40Z

csrc/executor.h


+  Fusion* fusion() const {
+    NVF_ERROR(
+        (lowered_ && !fusion_) || (!lowered_ && fusion_),


I think it's clearer to say

(lowered_ == nullptr) != (fusion_ == nullptr)

which essentially use != to do a logical xor.

wujingyue · 2024-04-26T18:16:48Z

csrc/executor.h

      int64_t group_id);

+  //! Check if compilation was skipped (fusion segment marked for EE).
+  bool isExprEval() const {


How about isExpressionEvaluated for less ambiguity?

wujingyue · 2024-04-26T18:24:57Z

tests/cpp/test_resize.cpp

+  preseg_passes::OptimizationPassGuard<preseg_passes::MarkAliasesPreparePass>
+      optimization_guard(false);
+
  auto fusion_ptr = std::make_unique<Fusion>();


I know you didn't add this, but since you are here, do you mind doing a minor, side refactor?

Change this line to
auto fusion = std::make_unique<Fusion>();

Remove the next line says auto& fusion=*fusion_ptr.

Change existing uses of fusion. to fusion->.

The gist is that unique_ptr comes with syntax sugar to make it as if it's a raw pointer. It's not necessary to create another variable just to hold its reference.

wujingyue mentioned this pull request Mar 15, 2024

Fix several problems related to perf_debug_verbose. #1945

Merged

Priya2698 force-pushed the pm/skip_compilation branch 2 times, most recently from bb2b202 to b853b91 Compare April 11, 2024 00:22

Priya2698 force-pushed the pm/skip_compilation branch from 94e037c to 015f0e5 Compare April 19, 2024 21:37

Priya2698 marked this pull request as ready for review April 22, 2024 20:41

Priya2698 requested review from jacobhinkle, jjsjann123, naoyam, rdspring1 and wujingyue April 22, 2024 20:41

Priya2698 commented Apr 22, 2024

View reviewed changes

Priya2698 linked an issue Apr 22, 2024 that may be closed by this pull request

Skip compiling fusion segments that are marked for EE #1812

Closed

wujingyue reviewed Apr 22, 2024

View reviewed changes

jjsjann123 reviewed Apr 22, 2024

View reviewed changes

Priya2698 added 9 commits April 24, 2024 20:33

test code

017d197

pm/skip_compilation

0737e29

add fusion_ptr_

c714373

remove mma aten eval workarounds

3ac1c82

directly evaluate outputs, add skip compilation method, use fusion in…

4c6f402

… tests

unify bind inputs code

77618c4

move error check

8f37e2e

add compilation check in tests

32e82a7

fix error condition, make copy of fusion

72dbd0c

Priya2698 and others added 14 commits April 24, 2024 20:33

modify tests

58cdc85

fix cpp tests

d50419e

fix lint

c1770ea

add iobyte compute

568004c

modify compilation_skip and compiled checks

c71eb2a

use fusion() instead of fusion_

15516f9

fix aten test

1469c57

lint

1d7ea65

Fix serialization errors when skipping compilation in `FusionExecutor…

c2a9d81

…`. (#2061) Serialization assumes that `FusionExecutor` always contains a compiled kernel. This PR updates serialization to skip compilation correctly.

Update csrc/executor.h

5f70982

Co-authored-by: Jingyue Wu <wujingyue@gmail.com>

Update csrc/executor.h

19d097f

Co-authored-by: jjsjann123 <jiej@nvidia.com>

review comments

2c73d6d

move checks to runFusion

3cf4a50

remove compilation flag from buffer

409d6c8

Priya2698 force-pushed the pm/skip_compilation branch from 2ac6cb3 to 409d6c8 Compare April 24, 2024 23:16

Priya2698 added 2 commits April 24, 2024 23:25

modify FE::isCompiled to denote whether compilation was attempted

b8d60f2

move check, rename

c27cc60

Priya2698 requested review from jjsjann123 and wujingyue April 25, 2024 03:18

simplify checks, lint

01a21fa

jjsjann123 reviewed Apr 25, 2024

View reviewed changes

csrc/executor.h Show resolved Hide resolved

csrc/executor.h Show resolved Hide resolved

csrc/executor.h Outdated Show resolved Hide resolved

csrc/executor.h Outdated Show resolved Hide resolved

jjsjann123 reviewed Apr 25, 2024

View reviewed changes

review comments

a76d154

jjsjann123 approved these changes Apr 25, 2024

View reviewed changes

Priya2698 merged commit 8ea9064 into main Apr 26, 2024

Priya2698 deleted the pm/skip_compilation branch April 26, 2024 16:57

wujingyue reviewed Apr 26, 2024

View reviewed changes

wujingyue mentioned this pull request Jan 17, 2025

Remove KernelExecutor::fusion_ #3725

Merged

		@@ -890,9 +890,7 @@ class PredicateChcker : public IterVisitor {
		} // namespace

		PredicateElimination::PredicateElimination(Fusion* fusion) {

Conversation

Priya2698 commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Priya2698 commented Mar 14, 2024

Uh oh!

Priya2698 commented Mar 14, 2024

Uh oh!

Priya2698 commented Apr 12, 2024

Uh oh!

Priya2698 commented Apr 15, 2024

Uh oh!

Priya2698 commented Apr 19, 2024

Uh oh!

Priya2698 commented Apr 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya2698 Apr 22, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya2698 Apr 23, 2024 • 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!

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!

Priya2698 commented Apr 24, 2024

Uh oh!

Uh oh!

Uh oh!

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!

jjsjann123 left a comment

Choose a reason for hiding this comment

Uh oh!

Priya2698 commented Apr 25, 2024

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Priya2698 commented Mar 13, 2024 •

edited

Loading

Priya2698 Apr 22, 2024 •

edited

Loading

Priya2698 Apr 23, 2024 •

edited

Loading