[Outlining] Remove overlapping sequences #7146

ashleynh · 2024-12-12T07:12:18Z

While determining whether repeat sequences of instructions are candidates for outlining, remove sequences that overlap, giving weight to sequences that are longer and appear more frequently.

tlively · 2025-01-18T01:30:54Z

src/support/intervals.h

+  bool operator<(const Interval& other) const {
+    return start < other.start && weight < other.weight;
+  }


This should take end into account as well. Otherwise the std::set<Interval> returned by IntervalProcessor::getOverlaps() will not be able to hold two intervals that differ only in their ends.

tlively · 2025-01-18T01:32:59Z

src/support/intervals.cpp

+
+std::set<Interval>
+IntervalProcessor::getOverlaps(std::vector<Interval>& intervals) {
+  std::sort(intervals.begin(), intervals.end(), [](Interval a, Interval b) {


Suggested change

std::sort(intervals.begin(), intervals.end(), [](Interval a, Interval b) {

std::sort(intervals.begin(), intervals.end(), [](const Interval& a, const Interval& b) {

Just to avoid copying intervals around unnecessarily.

tlively · 2025-01-18T01:33:46Z

src/support/intervals.cpp

+  });
+
+  std::set<Interval> overlaps;
+  auto& firstInterval = intervals[0];


There should be an early return if the input vector is empty to avoid UB here.

tlively · 2025-01-18T01:36:27Z

src/passes/hash-stringify-walker.cpp

+    for (auto startIdx : substring.StartIndices) {
+      auto interval =
+        Interval(startIdx,
+                 startIdx + substring.Length - 1,


I'm surprised we're using intervals inclusive of their ends. Would this work without the - 1 as well?

tlively · 2025-01-18T01:36:53Z

src/passes/hash-stringify-walker.cpp

+      auto interval =
+        Interval(startIdx,
+                 startIdx + substring.Length - 1,
+                 substring.Length * substring.StartIndices.size());


Probably worth a comment about why we are using this weight.

tlively · 2025-01-18T01:40:35Z

src/passes/hash-stringify-walker.cpp

+  std::set<Interval> overlaps = IntervalProcessor::getOverlaps(intervals);
+  std::set<unsigned> doNotInclude;
+  for (auto& interval : overlaps) {
+    doNotInclude.insert(intervalMap[interval]);
+  }


We could simplify the code here and get away without any map or set lookups if IntervalProcessor returned a sequence of kept indices in its input vector rather than a set of removed intervals. With a sequence of kept indices, we could directly construct the list of kept substrings.

great idea, thanks!

tlively · 2025-01-18T01:42:23Z

test/lit/passes/outlining.wast

  )
 )
+
+;; Test that no attempt is made to outline overlapping repeat substrings


It would be good to add comments about what the overlapping substrings are.

tlively · 2025-01-18T01:44:04Z

test/lit/passes/outlining.wast

+    (drop (i32.add
+      (i32.const 0)
+      (i32.const 1)
+    ))


We could make the test more concise and easy to read by just using constants and drops, unless I'm missing some reason why this wouldn't work.

(drop (i32.const 0)) (drop (i32.const 1)) (drop (i32.const 2)) (drop (i32.const 3)) (drop (i32.const 0)) (drop (i32.const 1)) (drop (i32.const 2)) (drop (i32.const 3)) (drop (i32.const 1)) (drop (i32.const 2)) (drop (i32.const 1)) (drop (i32.const 2))

good call, thanks

ashleynh · 2025-01-23T01:42:51Z

src/support/intervals.h

+};
+
+struct IntervalProcessor {
+  static std::set<Interval> getOverlaps(std::vector<Interval>&);


add gTests for edge cases

tlively

It would still be good to include gtest unit tests for the various kinds of overlaps.

tlively · 2025-01-30T19:27:13Z

src/passes/hash-stringify-walker.cpp

+  for (Index i = 0; i < substrings.size(); i++) {
+    auto substring = substrings[i];
+    for (auto startIdx : substring.StartIndices) {
+      // TODO: This weight was picked with an assumption


What assumption?

tlively · 2025-01-30T19:27:33Z

src/passes/Outlining.cpp

    DBG(printHashString(stringify.hashString, stringify.exprs));
    // Remove substrings that are substrings of longer repeat substrings.
    substrings = StringifyProcessor::dedupe(substrings);
+    // Remove substrings with overlapping indices


Suggested change

// Remove substrings with overlapping indices

// Remove substrings with overlapping indices.

tlively · 2025-01-30T19:28:27Z

src/passes/hash-stringify-walker.cpp

+  std::vector<Interval> intervals;
+  std::vector<int> substringIdxs;


It would be good to have a comment saying how these two vectors relate to each other.

tlively · 2025-01-30T19:36:27Z

src/passes/hash-stringify-walker.cpp

+    if (substringsIncluded.find(substringIdx) != substringsIncluded.end()) {
+      continue;
+    }
+    substringsIncluded.insert(substringIdx);


Suggested change

if (substringsIncluded.find(substringIdx) != substringsIncluded.end()) {

continue;

}

substringsIncluded.insert(substringIdx);

if (!substringsIncluded.insert(substringIdx)->second) {

continue;

}

tlively · 2025-01-30T19:37:24Z

src/support/intervals.h

+};
+
+struct IntervalProcessor {
+  // TODO: Given a vector of Interval, returns a vector of the indices, mapping


What's the TODO here?

it was for me to review the comment before submitting

tlively · 2025-01-30T21:09:17Z

src/passes/hash-stringify-walker.cpp

+    if (seenCount[substringIdx] == substring.StartIndices.size() &&
+        substringsIncluded.insert(substringIdx).second) {
+      result.push_back(substring);


So we're only considering a substring for outlining at all if all of its ocurrences survive overlap filtering? Could we keep the substring in consideration and just remove the particular occurrence of it that had the overlap instead?

tlively · 2025-01-30T21:11:43Z

src/support/intervals.cpp

  std::sort(
    intIntervals.begin(), intIntervals.end(), [](const auto& a, const auto& b) {
-      return a.first.start < b.first.end;
+      return a.first.start < b.first.start;


No need for the lambda here if you fix operator< to be a total order (meaning that for any pair of intervals a and b, exactly one of a < b,b < a, or a == b is true)

tlively · 2025-01-30T21:19:04Z

src/support/intervals.cpp


  std::vector<int> result;
-  auto& firstInterval = intIntervals[0];
+  auto& formerInterval = intIntervals[0];


Making this a reference means that when you do formerInterval = latterInterval below, it writes to the first element in intIntervals, which is a little odd. Intervals should be small enough that copying them is cheap, so let's just make this a non-reference. Alternatively, to avoid copying intervals, you could make this an index.

tlively · 2025-01-30T21:21:59Z

src/support/intervals.cpp

-      firstInterval = nextInterval;
-    } else {
-      result.push_back(firstInterval.second);
+    if (latterInterval.first.weight > formerInterval.first.weight) {


If the weights are equal, perhaps you can choose to keep the interval with the nearest end to reduce its potential to overlap with subsequent intervals.

tlively · 2025-01-30T21:22:50Z

src/support/intervals.h

-  // back to the original input vector, of non-overlapping indices, ie, the
-  // intervals that overlap have already been removed.
+  // Given a vector of Interval, returns a vector of the indices that, mapping
+  // back to the original input vector, do not overlap with each other, ie: the


Suggested change

// back to the original input vector, do not overlap with each other, ie: the

// back to the original input vector, do not overlap with each other, i.e. the

Doesn't look like it!

batched suggestion

tlively · 2025-01-30T21:26:03Z

test/gtest/intervals.cpp

+  std::vector<Interval> intervals;
+  intervals.emplace_back(Interval{0, 4, 2});
+  intervals.emplace_back(Interval{4, 8, 2});
+  ASSERT_EQ(IntervalProcessor::filterOverlaps(intervals).size(), 2u);


It would be better to test the precise results rather than just the size of the results. You can still do it with a single ASSERT_EQ:

std::vector<int> expected{0, 1}; ASSERT_EQ(IntervalProcessor::filterOverlaps(intervals), expected);

test/gtest/intervals.cpp

ashleynh · 2025-01-30T21:28:27Z

src/support/intervals.h

+
+struct IntervalProcessor {
+  // Given a vector of Interval, returns a vector of the indices that, mapping
+  // back to the original input vector, do not overlap with each other, ie: the


adjust punctuation around ie to , i.e.,

tlively · 2025-03-21T00:03:11Z

src/passes/hash-stringify-walker.cpp

+
+  // Construct intervals
+  for (Index i = 0; i < substrings.size(); i++) {
+    auto substring = substrings[i];


To avoid copying the list of start indices and whatever else is in there:

Suggested change

auto substring = substrings[i];

auto& substring = substrings[i];

batched suggestion

tlively · 2025-03-21T00:06:44Z

src/passes/hash-stringify-walker.cpp

+      result.push_back(SuffixTree::RepeatedSubstring(
+        {substrings[i].Length, std::move(startIndices[i])}));


Can this be an emplace_back to really be sure we're not copying the vector of start indices?

tlively · 2025-03-21T00:08:05Z

src/passes/hash-stringify-walker.cpp

+      auto interval =
+        Interval(startIdx, startIdx + substring.Length, substring.Length);
+      intervals.push_back(interval);


This could be slightly shorter using emplace_back.

tlively · 2025-03-21T00:08:50Z

src/support/intervals.cpp

+#include <iostream>
+#include <optional>


Looks like these are stale.

Suggested change

#include <iostream>

#include <optional>

batched suggestion

tlively · 2025-03-21T00:11:09Z

src/support/intervals.cpp

+    // former overlaps with candidate
+    // replace former if the weights are the same but the candidate ends earlier


Suggested change

// former overlaps with candidate

// replace former if the weights are the same but the candidate ends earlier

// Former overlaps with candidate. Replace former if the weights are the same but the candidate ends earlier.

batched suggestion

src/support/intervals.cpp

tlively · 2025-03-21T00:15:25Z

src/support/intervals.h

-  // back to the original input vector, of non-overlapping indices, ie, the
-  // intervals that overlap have already been removed.
+  // Given a vector of Interval, returns a vector of the indices that, mapping
+  // back to the original input vector, do not overlap with each other, ie: the


Doesn't look like it!

tlively · 2025-03-21T00:21:11Z

test/gtest/intervals.cpp

+  ASSERT_EQ(IntervalProcessor::filterOverlaps(intervals), expected);
+}
+
+TEST(IntervalsTest, TestOverlapFoundA) {


It would be best to give these more descriptive names like TestOverlapInnerNested, TestOverlapOuterNested, TestOverlapAscendingSequence, TestOverlapDescendingSequence, etc. Ideally the role each interval plays in a test should be clear from the name of the test.

tlively · 2025-03-21T00:24:12Z

test/lit/passes/outlining.wast

+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (i32.const 1)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (i32.const 2)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )


Since this substring isn't outlined at all, it's not clear to me that the test is behaving as expected. If we add more repetitions of this shorter substring, will it be outlined?

Co-authored-by: Thomas Lively <[email protected]>

tlively

LGTM with these last comments addressed.

tlively · 2025-03-25T19:44:47Z

src/passes/hash-stringify-walker.cpp

+      result.emplace_back(SuffixTree::RepeatedSubstring(
+        {substrings[i].Length, std::move(startIndices[i])}));


Suggested change

result.emplace_back(SuffixTree::RepeatedSubstring(

{substrings[i].Length, std::move(startIndices[i])}));

result.emplace_back(substrings[i].Length, std::move(startIndices[i]));

When I remove SuffixTree::RepeatedSubstring, I get the following error:

FAILED: src/passes/CMakeFiles/passes.dir/hash-stringify-walker.cpp.o /usr/bin/c++ -I/src -I/third_party/FP16/include -I/third_party/llvm-project/include -I -DBUILD_LLVM_DWARF -Wall -Werror -Wextra -Wno-unused-parameter -Wno-dangling-pointer -fno-omit-frame-pointer -fno-rtti -Wno-implicit-int-float-conversion -Wno-unknown-warning-option -Wswitch -Wimplicit-fallthrough -Wnon-virtual-dtor -fPIC -fdiagnostics-color=always -g -g3 -std=c++17 -MD -MT src/passes/CMakeFiles/passes.dir/hash-stringify-walker.cpp.o -MF src/passes/CMakeFiles/passes.dir/hash-stringify-walker.cpp.o.d -o src/passes/CMakeFiles/passes.dir/hash-stringify-walker.cpp.o -c /src/passes/hash-stringify-walker.cpp /src/passes/hash-stringify-walker.cpp: In static member function ‘static std::vector<wasm::SuffixTree::RepeatedSubstring> wasm::StringifyProcessor::filterOverlaps(const std::vector<wasm::SuffixTree::RepeatedSubstring>&)’: /src/passes/hash-stringify-walker.cpp:184:26: error: no matching function for call to ‘std::vector<wasm::SuffixTree::RepeatedSubstring>::emplace_back(<brace-enclosed initializer list>)’ 184 | result.emplace_back( | ~~~~~~~~~~~~~~~~~~~^ 185 | {substrings[i].Length, std::move(startIndices[i])}); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /usr/include/c++/14/vector:72, from /src/wasm.h:34, from /src/ir/boolean.h:20, from /src/ir/bits.h:20, from /src/ir/properties.h:20, from /src/ir/iteration.h:20, from /src/passes/stringify-walker.h:20, from /src/passes/hash-stringify-walker.cpp:17: /usr/include/c++/14/bits/vector.tcc:111:7: note: candidate: ‘std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::emplace_back(_Args&& ...) [with _Args = {}; _Tp = wasm::SuffixTree::RepeatedSubstring; _Alloc = std::allocator<wasm::SuffixTree::RepeatedSubstring>; reference = wasm::SuffixTree::RepeatedSubstring&]’ 111 | vector<_Tp, _Alloc>:: | ^~~~~~~~~~~~~~~~~~~ /usr/include/c++/14/bits/vector.tcc:111:7: note: candidate expects 0 arguments, 1 provided At global scope: cc1plus: note: unrecognized command-line option ‘-Wno-unknown-warning-option’ may have been intended to silence earlier diagnostics cc1plus: note: unrecognized command-line option ‘-Wno-implicit-int-float-conversion’ may have been intended to silence earlier diagnostics [3/18] Building CXX object src/passes/CMakeFiles/passes.dir/Outlining.cpp.o ninja: build stopped: subcommand failed.

no constructor provided on SuffixTree, so we'll keep as-is

tlively · 2025-03-25T19:50:41Z

test/gtest/intervals.cpp

+  intervals.emplace_back(Interval{12, 18, 3});
+  intervals.emplace_back(Interval{14, 21, 5});
+  intervals.emplace_back(Interval{23, 28, 5});
+  std::vector<int> expected{4, 3, 7};


Am I reading correctly that this is keeping {6, 20, 15} at index 4 followed by {4, 11, 1} at index 3? That doesn't seem right.

Indeed this expected vector was wrong and the test was failing. it is keeping {0, 4, 7}.

tlively · 2025-03-25T19:52:12Z

test/gtest/intervals.cpp

+
+TEST(IntervalsTest, TestOne) {
+  std::vector<Interval> intervals;
+  intervals.emplace_back(Interval{0, 2, 5});


Since we're using emplace_back, we can just pass the arguments to the constructor without having to construct and then move a temporary object.

Suggested change

intervals.emplace_back(Interval{0, 2, 5});

intervals.emplace_back(0, 2, 5);

tlively · 2025-03-25T19:56:58Z

test/gtest/intervals.cpp

+  intervals.emplace_back(Interval{2, 4, 2});
+  intervals.emplace_back(Interval{3, 6, 5});


By the name of the test, I would have expected to see an interval nested fully within another interval.

tlively

🎉 🎉 🎉

ashleynh force-pushed the intervals branch from 4c6c9f9 to 564e54e Compare December 19, 2024 22:58

ashleynh force-pushed the intervals branch 3 times, most recently from 1a589b6 to 0d961b8 Compare January 17, 2025 21:03

ashleynh changed the title ~~[*WIP* - Outlining] Remove overlapping sequences~~ [Outlining] Remove overlapping sequences Jan 17, 2025

ashleynh requested a review from tlively January 18, 2025 00:05

ashleynh marked this pull request as ready for review January 18, 2025 00:05

tlively reviewed Jan 18, 2025

View reviewed changes

ashleynh commented Jan 23, 2025

View reviewed changes

ashleynh added 6 commits January 23, 2025 19:06

non-overlapping intervals

ab65ba2

offby1

4636a3f

commit

aff6cd7

first pass addressing comments

76d31b0

reworked removeOverlaps

6a5aa0f

lint

b05f948

ashleynh force-pushed the intervals branch from 0d961b8 to b05f948 Compare January 23, 2025 19:26

ashleynh added 2 commits January 23, 2025 19:48

next

15485fa

set better

c68c921

tlively reviewed Jan 30, 2025

View reviewed changes

updates

956dbd0

tlively reviewed Jan 30, 2025

View reviewed changes

ashleynh commented Jan 30, 2025

View reviewed changes

ashleynh added 8 commits January 31, 2025 00:24

changes

6408a25

changing test

fd7c6c8

added tests

061211e

operator<

f828e45

greedily keeping overlapping intervals, more tests

a6bc337

comment

26d4ab7

updated tests

9ece5b6

new interval expected vectors

34cd8d1

lint and removing comment

54e1903

tlively reviewed Mar 21, 2025

View reviewed changes

ashleynh and others added 6 commits March 25, 2025 09:51

Apply suggestions from code review

b83bdb3

Co-authored-by: Thomas Lively <[email protected]>

emplace comments

116ee63

comment

d22fc3a

test names

79c0800

test comment

167dbf6

lint

381b2ce

tlively reviewed Mar 25, 2025

View reviewed changes

ashleynh added 4 commits March 25, 2025 20:25

lint

515d550

removing interval call

a044f24

inner

7d30725

fixing test

4f011f9

ashleynh requested a review from tlively March 25, 2025 22:49

tlively approved these changes Mar 26, 2025

View reviewed changes

ashleynh merged commit 1f01a77 into main Mar 26, 2025
14 checks passed

ashleynh deleted the intervals branch March 26, 2025 00:40

	std::sort(intervals.begin(), intervals.end(), [](Interval a, Interval b) {
	std::sort(intervals.begin(), intervals.end(), [](const Interval& a, const Interval& b) {

	// Remove substrings with overlapping indices
	// Remove substrings with overlapping indices.

		std::vector<Interval> intervals;
		std::vector<int> substringIdxs;

	// back to the original input vector, do not overlap with each other, ie: the
	// back to the original input vector, do not overlap with each other, i.e. the

	auto substring = substrings[i];
	auto& substring = substrings[i];

		result.push_back(SuffixTree::RepeatedSubstring(
		{substrings[i].Length, std::move(startIndices[i])}));

		// former overlaps with candidate
		// replace former if the weights are the same but the candidate ends earlier

	// former overlaps with candidate
	// replace former if the weights are the same but the candidate ends earlier
	// Former overlaps with candidate. Replace former if the weights are the same but the candidate ends earlier.

		result.emplace_back(SuffixTree::RepeatedSubstring(
		{substrings[i].Length, std::move(startIndices[i])}));

	result.emplace_back(SuffixTree::RepeatedSubstring(
	{substrings[i].Length, std::move(startIndices[i])}));
	result.emplace_back(substrings[i].Length, std::move(startIndices[i]));

	intervals.emplace_back(Interval{0, 2, 5});
	intervals.emplace_back(0, 2, 5);

		intervals.emplace_back(Interval{2, 4, 2});
		intervals.emplace_back(Interval{3, 6, 5});

[Outlining] Remove overlapping sequences #7146

[Outlining] Remove overlapping sequences #7146

Uh oh!

Conversation

ashleynh commented Dec 12, 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!

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!

Choose a reason for hiding this comment

Uh oh!

ashleynh Jan 23, 2025 • 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!

tlively 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!

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

ashleynh Jan 23, 2025 •

edited

Loading