[SLPVectorizer] Clear TreeEntryToStridedPtrInfoMap.

[mgudim]

We need to clear TreeEntryToStridedPtrInfoMap in deleteTree.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Mikhail Gudim (mgudim)

Changes

We need to clear TreeEntryToStridedPtrInfoMap in deleteTree.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp (+1)

diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index 1814d9a6811c0..39530e92e692d 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -2099,6 +2099,7 @@ class BoUpSLP {
     UserIgnoreList = nullptr;
     PostponedGathers.clear();
     ValueToGatherNodes.clear();
+    TreeEntryToStridedPtrInfoMap.clear();
   }
 
   unsigned getTreeSize() const { return VectorizableTree.size(); }

[alexey-bataev]

The test is required. Or revert the original commit and fix it there

[mgudim]

The test is required. Or revert the original commit and fix it there

What kind of test should it be?

Reverting seems more complicated because several other merged commits were based on this one.

[alexey-bataev]

The one that reveals the issue with not clearing the map

[mgudim]

The one that reveals the issue with not clearing the map

The issue would be revealed only if we use the same BoUpSLP and when building a tree some memory is reused for a TreeNode. I don't see a way to set this up except a unit test and even that would be non-trivial. There is no test for any of lines is deleteTree as far as I see.

Should I start a new test in unittests to test SLPVectrorizer?

[alexey-bataev]

The one that reveals the issue with not clearing the map

The issue would be revealed only if we use the same BoUpSLP and when building a tree some memory is reused for a TreeNode. I don't see a way to set this up except a unit test and even that would be non-trivial. There is no test for any of lines is deleteTree as far as I see.

Should I start a new test in unittests to test SLPVectrorizer?

Up to you, it can be a unittest or a lit test, that crashes for you.

[mgudim]

Up to you, it can be a unittest or a lit test, that crashes for you.

Would it be ok that in a different commit I'll add asserts in "buildTree" that make sure everything from deleteTree is empty except TreeEntryToStridedPtrInfoMap. In this commit I'll add the assert and clear for the TreeEntryToStridedPtrInfoMap. This way it's a test every time buildTree is called!

[alexey-bataev]

Ok, but still need a test

[mgudim]

Why do we need a test if we have asserts? Don't asserts make it self-evident that the map is cleared? The test would essentially be the same this as an assert, just much more effort to set up.

[alexey-bataev]

Each non-nfc patch requires a test

[mgudim]

I am out of ideas.

I don't see how to add asserts to the code so it doesn't look stupid. Current code is like this:

"buildTree(...) {
deleteTree(...);
...
buildTreeRec(...)
}
"
adding asserts after deleteTree is like this code:

int x = 2;
assert (x == 2);

I thought that maybe I can set up a unit test. There is no public header to include which would contain BoUpSLP.

Constructing test by hand to try to make SLP crash is very hard.

There won't be a memory leak because the map gets deleted automatically with BoUpSLP object so setting up some kind of valgrind - based test is also not an option.

I can measure the size of BoUpSLP object right after it's created and right after deleteTree and assert that they are equal. Test would check that the assert is not hit. Will this be acceptable?

Can you suggest something else?

[mgudim]

Each non-nfc patch requires a test

Is this an official rule written somewhere? I'm pretty sure that any reasonable guidelines would allow merging something like this without a test.

[mgudim]

I can measure the size of BoUpSLP object right after it's created and right after deleteTree and assert that they are equal. Test would check that the assert is not hit. Will this be acceptable?

ping

[alexey-bataev]

Just add a test that crahes

[mgudim]

I can't come up with a test that crashes

[alexey-bataev]

Simple test that crashes with the assertion but without clearing the map

[mgudim]

sorry, I don't understand what you mean. If I add an assertion somewhere that map is not cleared, it will break llvm for everyone until the commit which clears the map is merged

[alexey-bataev]

I understand it. Just add a new test, that will crash with assertion but without clearing.

[mgudim]

I understand it. Just add a new test, that will crash with assertion but without clearing.

Is this what you mean:

#161227

[github-actions]

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

[alexey-bataev]

Add real checks here