-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[SandboxVectorizer] Add container class to track and manage SeedBundles #112048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
ca37498
e9d7a34
a12d450
a5646c5
5fcbbaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -54,6 +54,10 @@ class SeedBundle { | |||||
| NumUnusedBits += Utils::getNumBits(I); | ||||||
| } | ||||||
|
|
||||||
| virtual void insert(Instruction *I, ScalarEvolution &SE) { | ||||||
| assert("Subclasses must override this function."); | ||||||
| } | ||||||
|
|
||||||
| unsigned getFirstUnusedElementIdx() const { | ||||||
| for (unsigned ElmIdx : seq<unsigned>(0, Seeds.size())) | ||||||
| if (!isUsed(ElmIdx)) | ||||||
|
|
@@ -96,6 +100,9 @@ class SeedBundle { | |||||
| MutableArrayRef<Instruction *> | ||||||
| getSlice(unsigned StartIdx, unsigned MaxVecRegBits, bool ForcePowOf2); | ||||||
|
|
||||||
| /// \Returns the number of seed elements in the bundle. | ||||||
| std::size_t size() const { return Seeds.size(); } | ||||||
|
|
||||||
| protected: | ||||||
| SmallVector<Instruction *> Seeds; | ||||||
| /// The lanes that we have already vectorized. | ||||||
|
|
@@ -148,7 +155,7 @@ template <typename LoadOrStoreT> class MemSeedBundle : public SeedBundle { | |||||
| "Expected LoadInst or StoreInst!"); | ||||||
| assert(isa<LoadOrStoreT>(MemI) && "Expected Load or Store!"); | ||||||
| } | ||||||
| void insert(sandboxir::Instruction *I, ScalarEvolution &SE) { | ||||||
| void insert(sandboxir::Instruction *I, ScalarEvolution &SE) override { | ||||||
| assert(isa<LoadOrStoreT>(I) && "Expected a Store or a Load!"); | ||||||
| auto Cmp = [&SE](Instruction *I0, Instruction *I1) { | ||||||
| return Utils::atLowerAddress(cast<LoadOrStoreT>(I0), | ||||||
|
|
@@ -162,5 +169,108 @@ template <typename LoadOrStoreT> class MemSeedBundle : public SeedBundle { | |||||
| using StoreSeedBundle = MemSeedBundle<sandboxir::StoreInst>; | ||||||
| using LoadSeedBundle = MemSeedBundle<sandboxir::LoadInst>; | ||||||
|
|
||||||
| /// Class to conveniently track Seeds within Seedbundles. Saves newly collected | ||||||
|
||||||
| /// Class to conveniently track Seeds within Seedbundles. Saves newly collected | |
| /// Class to conveniently track Seeds within SeedBundles. Saves newly collected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completely used -> completely unused ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterators skip bundles that are completely used, because the vectorizer will set an instruction as "used" when it is vectorized (or we have concluded that it can't be vectorized). We don't actually remove them from the bundle so that "removal" is a constant time operation. I will elaborate on this in the iterator comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes you are referring to the used flag. I was thinking of something tagged with the used flag as unused.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop sandboxir::
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a brief description, mentioning what it iterates over and whether the iterator follows a specific order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sterling-Augustine I'm seeing "comparison of unsigned expression >= 0 is always true" warnings due to VecIdx being unsigned - please can you take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #112392
Sorry for the trouble
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,10 @@ | |
| using namespace llvm; | ||
| namespace llvm::sandboxir { | ||
|
|
||
| cl::opt<unsigned> SeedBundleSizeLimit( | ||
| "sbvec-seed-bundle-size-limit", cl::init(32), cl::Hidden, | ||
| cl::desc("Limit the size of the seed bundle to cap compilation time.")); | ||
|
|
||
| MutableArrayRef<Instruction *> SeedBundle::getSlice(unsigned StartIdx, | ||
| unsigned MaxVecRegBits, | ||
| bool ForcePowerOf2) { | ||
|
|
@@ -61,4 +65,73 @@ MutableArrayRef<Instruction *> SeedBundle::getSlice(unsigned StartIdx, | |
| return {}; | ||
| } | ||
|
|
||
| template <typename LoadOrStoreT> | ||
| SeedContainer::KeyT SeedContainer::getKey(LoadOrStoreT *LSI) const { | ||
| assert((isa<LoadInst>(LSI) || isa<StoreInst>(LSI)) && | ||
| "Expected Load or Store!"); | ||
| Value *Ptr = Utils::getMemInstructionBase(LSI); | ||
| Instruction::Opcode Op = LSI->getOpcode(); | ||
| Type *Ty = Utils::getExpectedType(LSI); | ||
| if (auto *VTy = dyn_cast<VectorType>(Ty)) | ||
| Ty = VTy->getElementType(); | ||
| return {Ptr, Ty, Op}; | ||
| } | ||
|
|
||
| // Explicit instantiations | ||
| template SeedContainer::KeyT | ||
| SeedContainer::getKey<LoadInst>(LoadInst *LSI) const; | ||
| template SeedContainer::KeyT | ||
| SeedContainer::getKey<StoreInst>(StoreInst *LSI) const; | ||
|
|
||
| bool SeedContainer::erase(Instruction *I) { | ||
| assert((isa<LoadInst>(I) || isa<StoreInst>(I)) && "Expected Load or Store!"); | ||
| auto It = SeedLookupMap.find(I); | ||
| if (It == SeedLookupMap.end()) | ||
| return false; | ||
| SeedBundle *Bndl = It->second; | ||
| Bndl->setUsed(I); | ||
| return true; | ||
| } | ||
|
|
||
| template <typename LoadOrStoreT> void SeedContainer::insert(LoadOrStoreT *LSI) { | ||
| // Find the bundle containing seeds for this symbol and type-of-access. | ||
| auto &BundleVec = Bundles[getKey(LSI)]; | ||
| // Fill this vector of bundles front to back so that only the last bundle in | ||
| // the vector may have available space. This avoids iteration to find one with | ||
| // space. | ||
| if (BundleVec.empty() || BundleVec.back()->size() == SeedBundleSizeLimit) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will this work when instructions are inserted in out-of-order offsets? Can you move instructions between bundles to make space for one that "completes" another bundle, or do you avoid this by pre-sorting by offsets before insertion (I assumed this is not the case due to the logic in insert checking
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normally this won't be a problem. Seeds are inserted into the bundle at the proper offset position in memory by the calculations in insert and then calling insertAt. But you are right that if we exceed the seed count limit for a bundle, we could encounter a situation where a vectorizable group isn't found because it is split across two bundles. The limit is either 32 loads or stores with the same base symbol (a nice power-of-two, so hopefully vectorizable). I wouldn't expect to encounter it frequently for now, but long-term this is a problem that will need to be fixed, as noted in the TODO on line 184. Perhaps I should expand that explanation.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SG. |
||
| BundleVec.emplace_back(std::make_unique<MemSeedBundle<LoadOrStoreT>>(LSI)); | ||
| else | ||
| BundleVec.back()->insert(LSI, SE); | ||
|
|
||
| SeedLookupMap[LSI] = BundleVec.back().get(); | ||
| this->dump(); | ||
| } | ||
|
|
||
| // Explicit instantiations | ||
| template void SeedContainer::insert<LoadInst>(LoadInst *); | ||
| template void SeedContainer::insert<StoreInst>(StoreInst *); | ||
|
|
||
| #ifndef NDEBUG | ||
| void SeedContainer::dump(raw_ostream &OS) const { | ||
| for (const auto &Pair : Bundles) { | ||
| auto [I, Ty, Opc] = Pair.first; | ||
| const auto &SeedsVec = Pair.second; | ||
| std::string RefType = dyn_cast<LoadInst>(I) ? "Load" | ||
| : dyn_cast<StoreInst>(I) ? "Store" | ||
| : "Other"; | ||
| OS << "[Inst=" << *I << " Ty=" << Ty << " " << RefType << "]\n"; | ||
| for (const auto &SeedPtr : SeedsVec) { | ||
| SeedPtr->dump(OS); | ||
| OS << "\n"; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void SeedContainer::dump() const { | ||
| dump(dbgs()); | ||
| dbgs() << "\n"; | ||
| } | ||
| #endif // NDEBUG | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just have a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do in the future. |
||
|
|
||
| } // namespace llvm::sandboxir | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= 0?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated about that, but making the function pure virtual means that the top-level class can't be directly instantiated and therefore can't be tested independently, and most of the functionality is in this class, not the subclasses.
I suppose I could make the test subclass it with a stub implementation, but that seems like extra complexity for not much gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm_unreachable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you debate about
=0, assert, andllvm_unreachable, how about introducingSeedBundleBaseandSeedBundle? Then you can have the=0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would solve the problem. Right now
insertisn't pure virtual so that the tests for SeedBundle (which has a fair amount of code) can test an actual SeedBundle. If SeedBundleBase has a pure virtual function, it would have the same untestable problem--and all the useful code would be inside it. That's basically just renaming the class. There needs to be a stub somewhere so it can be tested independently.I suppose I'll make it pure virtual, and then make the test itself locally instantiate a "SeedBundleForTest" with the function overridden to a stub that looks exactly like the one now. I personally don't think that is appreciably better than an error, but whatever.