AliasAnalysis handles slices. by wujingyue · Pull Request #1281 · NVIDIA/Fuser

wujingyue · 2023-11-11T16:21:28Z

No description provided.

This is separated from #1281 to keep the PR small.

wujingyue · 2023-11-13T06:12:24Z

!build

wujingyue · 2023-11-13T06:13:55Z

!build

wujingyue · 2023-11-13T06:36:39Z

!build

csrc/optimization/alias_analysis.cpp

This is separated from #1281 to keep the PR small.

naoyam · 2023-11-13T20:02:33Z

Can this be split to two PRs, one for slices and another for reshapes?

wujingyue · 2023-11-13T21:35:03Z

Can this be split to two PRs, one for slices and another for reshapes?

Sure. Found more problems though: apparently, the pointwise scheduler changes the output contiguity, breaking ResizeTest.SliceAndReshape2. Looking...

wujingyue · 2023-11-13T22:06:31Z

Can this be split to two PRs, one for slices and another for reshapes?

Sure. Found more problems though: apparently, the pointwise scheduler changes the output contiguity, breaking ResizeTest.SliceAndReshape2. Looking...

This is the apparent root cause:

Fuser/csrc/tensor_view.cpp

Line 1210 in 2d2a2a4

TensorView* new_output = IrBuilder::create<TensorView>(

doesn't copy allocation domain and contiguity.

wujingyue · 2023-11-14T02:46:12Z

Sure. Found more problems though: apparently, the pointwise scheduler changes the output contiguity, breaking ResizeTest.SliceAndReshape2. Looking...

This is the apparent root cause:

Fuser/csrc/tensor_view.cpp

Line 1210 in 2d2a2a4

TensorView* new_output = IrBuilder::create<TensorView>(

doesn't copy allocation domain and contiguity.

Will be fixed by #1291

wujingyue · 2023-11-14T05:42:32Z

!build

jacobhinkle · 2023-11-14T08:59:54Z

csrc/optimization/alias_analysis.cpp

+  }
+
+  // Scan through the allocation domain in minor-to-major order. If an
+  // IterDomain is sliced, the next non-broadcast IterDomain has to be marked


Is it possible to encounter a trivial SliceOp here? Where both left and right pad are zero? We should treat such case as the identity on that axis.

Agree with Jacob. i.e. if we later decide to merge a trivially sliced dimension, we should recognize that and merge it back I guess.

But this can only be done opportunistically (only handle static trivial slice), I guess?! Since padding value can be a runtime value and we can't know for sure that it'll be a trivial slice or not until runtime.

Is it possible to encounter a trivial SliceOp here? Where both left and right pad are zero? We should treat such case as the identity on that axis.

Fuser/csrc/ops/alias.cpp

Line 792 in 2e2f3c7

return set(inp);

and

Fuser/csrc/ops/alias.cpp

Line 772 in 2e2f3c7

// This dim doesn't need slicing

have already done a good job filtering out trivial slices and trivial slice dimensions. So I'd rather keep the logic here simple.

jacobhinkle · 2023-11-14T09:00:48Z

csrc/optimization/alias_analysis.cpp

+      out_layout.contiguity[i] = in_layout.contiguity[i];
+    }
+
+    std::vector<Expr*> dependencies = DependencyCheck::getAllExprsBetween(


You can use StmtSort::getExprsBetween.

jacobhinkle

This PR looks good to me. Note that a slice can also be achieved with a PadOp. In that case, we cannot assume the pads are negative, and if any are positive we must materialize. Otherwise we can alias just as you've done here.

wujingyue · 2023-11-14T17:44:53Z

This PR looks good to me. Note that a slice can also be achieved with a PadOp. In that case, we cannot assume the pads are negative, and if any are positive we must materialize. Otherwise we can alias just as you've done here.

Is the reverse also true? I.e. a SliceOp may take a negative offset, making it essentially a pad. That would break my analysis because a real pad has to be materialized.

jjsjann123 · 2023-11-14T18:43:38Z

Should I review this one or wait until we split the PR into two per Naoya's suggestion?

wujingyue · 2023-11-14T18:44:33Z

Should I review this one or wait until we split the PR into two per Naoya's suggestion?

Already split

csrc/optimization/alias_analysis.cpp

jjsjann123 · 2023-11-14T23:51:44Z

csrc/optimization/alias_analysis.cpp

+  }
+
+  // Scan through the allocation domain in minor-to-major order. If an
+  // IterDomain is sliced, the next non-broadcast IterDomain has to be marked


Agree with Jacob. i.e. if we later decide to merge a trivially sliced dimension, we should recognize that and merge it back I guess.

But this can only be done opportunistically (only handle static trivial slice), I guess?! Since padding value can be a runtime value and we can't know for sure that it'll be a trivial slice or not until runtime.

test/test_alias.cpp

jacobhinkle · 2023-11-15T01:09:53Z

This PR looks good to me. Note that a slice can also be achieved with a PadOp. In that case, we cannot assume the pads are negative, and if any are positive we must materialize. Otherwise we can alias just as you've done here.

Is the reverse also true? I.e. a SliceOp may take a negative offset, making it essentially a pad. That would break my analysis because a real pad has to be materialized.

No. Slice will never pad. See #460.

wujingyue · 2023-11-16T01:04:08Z

This PR looks good to me. Note that a slice can also be achieved with a PadOp. In that case, we cannot assume the pads are negative, and if any are positive we must materialize. Otherwise we can alias just as you've done here.

Left a note on AliasFinder:handle(const SliceOp* slice) for future improvement.

jjsjann123

LGTM

Partially taken from #1281 to keep that PR small.

With slices, the pointer address might not be the same. PointerArithmetic is more accurate.

wujingyue · 2023-11-17T19:27:21Z

!build

This reverts commit eef9db8.

This reverts commit 64a21ae.

wujingyue marked this pull request as draft November 11, 2023 16:21

wujingyue added a commit that referenced this pull request Nov 12, 2023

Add LinkedHashMap and unit tests.

51402b1

This is separated from #1281 to keep the PR small.

wujingyue added a commit that referenced this pull request Nov 12, 2023

Add LinkedHashMap and unit tests.

bcd3b3a

This is separated from #1281 to keep the PR small.

wujingyue mentioned this pull request Nov 12, 2023

Add LinkedHashMap and unit tests. #1282

Merged

wujingyue added the enhancement label Nov 12, 2023

wujingyue changed the base branch from main to wjy/map November 13, 2023 04:34

wujingyue added a commit that referenced this pull request Nov 13, 2023

Add LinkedHashMap and unit tests.

0476f8d

This is separated from #1281 to keep the PR small.

wujingyue force-pushed the wjy/map branch from bcd3b3a to 0476f8d Compare November 13, 2023 04:36

wujingyue force-pushed the wjy/slice branch from d8dfbe7 to bef5a35 Compare November 13, 2023 04:37

wujingyue marked this pull request as ready for review November 13, 2023 06:12

wujingyue requested review from jacobhinkle, jjsjann123 and naoyam November 13, 2023 06:12

wujingyue commented Nov 13, 2023

View reviewed changes

csrc/optimization/alias_analysis.cpp Outdated Show resolved Hide resolved

Base automatically changed from wjy/map to main November 13, 2023 18:38

wujingyue added a commit that referenced this pull request Nov 13, 2023

Add LinkedHashMap and unit tests. (#1282)

f8d8435

This is separated from #1281 to keep the PR small.

wujingyue force-pushed the wjy/slice branch from afe63f7 to dc82092 Compare November 14, 2023 02:26

wujingyue mentioned this pull request Nov 14, 2023

AliasAnalysis handles non-packed reshapes. #1295

Merged

wujingyue force-pushed the wjy/slice branch from 008acbd to 87cd15c Compare November 14, 2023 05:40

wujingyue changed the base branch from main to wjy/nonpack November 14, 2023 05:41

wujingyue changed the title ~~AliasAnalysis handles slices and non-packed reshapes.~~ AliasAnalysis handles slices. Nov 14, 2023

jacobhinkle reviewed Nov 14, 2023

View reviewed changes

jacobhinkle approved these changes Nov 14, 2023

View reviewed changes

jjsjann123 reviewed Nov 14, 2023

View reviewed changes

wujingyue force-pushed the wjy/nonpack branch from 61ef979 to 9e8cdd2 Compare November 16, 2023 00:10

wujingyue force-pushed the wjy/slice branch from 45185df to d911358 Compare November 16, 2023 01:32

wujingyue requested a review from jjsjann123 November 16, 2023 01:33

jjsjann123 approved these changes Nov 16, 2023

View reviewed changes

wujingyue mentioned this pull request Nov 17, 2023

TransformReplay::replayCasP sets the right contiguity. #1291

Merged

wujingyue force-pushed the wjy/nonpack branch from 4dc97e8 to 3e8c98f Compare November 17, 2023 18:22

Base automatically changed from wjy/nonpack to main November 17, 2023 19:22

wujingyue added a commit that referenced this pull request Nov 17, 2023

AliasAnalysis handles non-packed reshapes. (#1295)

801670e

Partially taken from #1281 to keep that PR small.

wujingyue added 4 commits November 17, 2023 11:26

AliasAnalysis handles SliceOps.

6bf93d8

More comments.

46c9e6d

More tests.

01f0cde

s/PointerCast/PointerArithmetic. (#1321)

ebe2689

With slices, the pointer address might not be the same. PointerArithmetic is more accurate.

wujingyue force-pushed the wjy/slice branch from bdd4853 to ebe2689 Compare November 17, 2023 19:26

wujingyue merged commit eef9db8 into main Nov 17, 2023

wujingyue deleted the wjy/slice branch November 17, 2023 20:36

wujingyue added a commit that referenced this pull request Nov 20, 2023

Revert "AliasAnalysis handles slices. (#1281)"

64a21ae

This reverts commit eef9db8.

wujingyue added a commit that referenced this pull request Nov 20, 2023

Roll forward "AliasAnalysis handles slices. (#1281)".

4f76de2

This reverts commit 64a21ae.

wujingyue added a commit that referenced this pull request Nov 20, 2023

Revert "AliasAnalysis handles slices. (#1281)" (#1355)

f4c4d86

wujingyue added a commit that referenced this pull request Nov 21, 2023

Roll forward "AliasAnalysis handles slices. (#1281)".

df8859d

This reverts commit 64a21ae.

wujingyue added a commit that referenced this pull request Nov 21, 2023

Roll forward "AliasAnalysis handles slices. (#1281)". (#1357)

3b9f421

Conversation

wujingyue commented Nov 11, 2023

Uh oh!

wujingyue commented Nov 13, 2023

Uh oh!

wujingyue commented Nov 13, 2023

Uh oh!

wujingyue commented Nov 13, 2023

Uh oh!

Uh oh!

naoyam commented Nov 13, 2023

Uh oh!

wujingyue commented Nov 13, 2023

Uh oh!

wujingyue commented Nov 13, 2023

Uh oh!

wujingyue commented Nov 14, 2023

Uh oh!

wujingyue commented Nov 14, 2023

Uh oh!

jacobhinkle Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

jjsjann123 Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

wujingyue Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

jacobhinkle Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

jacobhinkle left a comment

Choose a reason for hiding this comment

Uh oh!

wujingyue commented Nov 14, 2023

Uh oh!

jjsjann123 commented Nov 14, 2023

Uh oh!

wujingyue commented Nov 14, 2023

Uh oh!

Uh oh!

jjsjann123 Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobhinkle commented Nov 15, 2023

Uh oh!

wujingyue commented Nov 16, 2023

Uh oh!

jjsjann123 left a comment

Choose a reason for hiding this comment

Uh oh!

wujingyue commented Nov 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants