[PATCH] D149893: Rewrite LSV to handle longer chains.
Artem Belevich via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 16 16:23:38 PDT 2023
tra added a comment.
I went through the code, but didn't look at the test changes yet. While I have a reasonable idea of what's going on, a lot of the patch details went over my head.
Overall, the patch looks OK to me, with mostly minor style/comment nits and a few clarification questions.
Do you have any data on how much impact on compilation time we'll see with this patch?
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:207-208
+
+/// Reorders the instructions that I depends on (the instructions defining its
+/// operands), to ensure they dominate I.
+void reorder(Instruction *I) {
----------------
Naive question -- shouldn't this already be the case for the instructions within the same BB?
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:455-456
+ // can make the equivalence class keys point to freed memory. This is
+ // fixable, but it's simpler just to wait until we're done with the BB and
+ // erase all at once.
+ for (Instruction *I : ToErase) {
----------------
This should probably be mentioned at the point where ToErase field is declared. I was wondering why it was not a local.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:458
+ for (Instruction *I : ToErase) {
+ auto *GEP = dyn_cast<GetElementPtrInst>(getLoadStorePointerOperand(I));
+ if (I->use_empty())
----------------
Is there a particular reason GEPs are special cased here? Dont we want to erase other unused arguments of the instructions we're deleting? Are we guaranteed that GEPs are the only argument kinds we're dealing with here? E.g. we may conceivably have an `inttoptr`.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:558-569
+ auto ChainBegin = [&] {
+ if constexpr (IsLoad())
+ return C.begin();
+ else
+ return C.rbegin();
+ }();
+ auto ChainEnd = [&] {
----------------
could it be merged into something like this:
```
auto [ChainBegin, ChainEnd] = [&] {
if constexpr (IsLoad())
return {C.begin(), C.end()};
else
return {C.rbegin(), C.rend()};
}();
```
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:689-690
+ // - Starting from the longest prefix, try to create a vector of that length.
+ // - If one of them works, great. Recurse on any remaining elements in the
+ // chain.
+ // - If none of them work, discard the first element and recurse on a chain
----------------
Is the comment still valid? I'm failing to find where the recursion happens in the code.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:795
+ << AS << " with alignment " << Alignment.value()
+ << " is has relative speed " << VectorizedSpeed
+ << ", which is lower than the elementwise speed of "
----------------
Nit: drop `is`
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:798
+ << ElementwiseSpeed
+ << ". Therefore this access can't be vectorized.\n");
+ return false;
----------------
Nit: "should not" or "will not" would probably work better.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1175
-bool Vectorizer::lookThroughComplexAddresses(Value *PtrA, Value *PtrB,
- APInt PtrDelta,
- unsigned Depth) const {
+std::optional<APInt> Vectorizer::lookThroughComplexAddresses(Value *PtrA,
+ Value *PtrB,
----------------
It's not obvious what the optional APInt represents here. Perhaps the function needs a better name to reflect it.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1289
-bool Vectorizer::lookThroughSelects(Value *PtrA, Value *PtrB,
- const APInt &PtrDelta,
- unsigned Depth) const {
+std::optional<APInt> Vectorizer::lookThroughSelects(Value *PtrA, Value *PtrB,
+ unsigned Depth) {
----------------
Ditto.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1338
- ++NumFound;
- if (NumFound == 1) {
- FirstInstr = I.getIterator();
- }
- if (NumFound == Chain.size()) {
- LastInstr = I.getIterator();
- break;
- }
- }
+ assert((LI != nullptr) != (SI != nullptr));
----------------
We already know that we can't have them both be nullptr.
So the only case when the assertion fill be false is when they both are non-null, and that is not possible as I can'be both a load and a store.
Drop it?
We could also change the code a bit to make the intent a bit more obvious:
```
auto *LI = dyn_cast<LoadInst>(&I);
auto *SI = !LI ? dyn_cast<StoreInst>(&I) : nullptr;
if (!LI && !SI)
continue;
```
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:1438-1439
- // We can ignore the alias if the we have a load store pair and the load
- // is known to be invariant. The load cannot be clobbered by the store.
- auto IsInvariantLoad = [](const LoadInst *LI) -> bool {
- return LI->hasMetadata(LLVMContext::MD_invariant_load);
- };
+ // Compare each instruction in `instrs` to leader of the N most recently-used
+ // chains. This limits the O(n^2) behavior of this pass while also allowing
+ // us to build arbitrarily long chains.
----------------
There's a bit of a dissonance between `LRU` and the "most recently used".
AFAICT the intent here is the access policy, and looking at the code it appears to be that "most recently used" is correct.
Rename LRU -> MRU?
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:169
+}
+void sortChainInOffsetOrder(Chain &C) {
+ sort(C, [](const auto &A, const auto &B) {
----------------
Nit: empty line between functions
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:285
+ /// Splits the chain into subchains of instructions which read/write a
+ /// contiguous block of memory. Doesn't return length-1 chains.
+ std::vector<Chain> splitChainByContiguity(Chain &C);
----------------
This raises more questions:
- What *does* it return if it's given a 1-element chain?
- Is it allowed not to split a chain (e.g if it's passed a 2-element chain)?
- If it's given a 3-element chain, does it mean that it will return the input unchanged? What if we do want to split the chain. E.g. if we have 3 64-bit loads, for NVPTX it would be optimal to vectorize them into v2i64 and i64.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:294
+ /// Splits the chain into subchains that make legal, aligned accesses.
+ /// Doesn't return length-1 chains.
+ std::vector<Chain> splitChainByAlignment(Chain &C);
----------------
"Doesn't return length-1 chains." seems to be a common pattern. It would help to elaborate on that in one place. E.g. something along the lines of "all chain split operations expect at least 2 elements long input chain and will never produce singleton chains as the output. In case input chain can not be split, the original chain is returned." Note, that I didn't look at the implementation yet, so the details are probably wrong, edit as needed.
================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:477
+ LLVM_DEBUG({
+ dbgs() << "LSV: Running on pseudo-BB [" << *Begin << ", ";
+ if (End != Begin->getParent()->end())
----------------
Nit: `...` may be a better indication that we're dealing with a range. Comma may be somewhat confusing considering that it will be present in the IR instructions themselves.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149893/new/
https://reviews.llvm.org/D149893
More information about the llvm-commits
mailing list