-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[SandboxVectorizer][NFCI] Fix use of possibly-uninitialized scalar. #122201
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 4 commits
807217e
9344694
3c8349c
11901e0
803fc9c
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 |
|---|---|---|
|
|
@@ -18,7 +18,8 @@ | |
|
|
||
| #include <cstdint> | ||
|
|
||
| namespace llvm::sandboxir { | ||
| namespace llvm { | ||
| namespace sandboxir { | ||
|
|
||
| class Argument; | ||
| class BBIterator; | ||
|
|
@@ -37,10 +38,25 @@ class Context { | |
| using MoveInstrCallback = | ||
| std::function<void(Instruction *, const BBIterator &)>; | ||
|
|
||
| /// An ID for a registered callback. Used for deregistration. Using a 64-bit | ||
| /// integer so we don't have to worry about the unlikely case of overflowing | ||
| /// a 32-bit counter. | ||
| using CallbackID = uint64_t; | ||
| /// An ID for a registered callback. Used for deregistration. A dedicated type | ||
| /// is employed so as to keep IDs opaque to the end user; only Context should | ||
| /// deal with its underlying representation. | ||
| class CallbackID { | ||
| public: | ||
| // Uses a 64-bit integer so we don't have to worry about the unlikely case | ||
| // of overflowing a 32-bit counter. | ||
| using ReprTy = uint64_t; | ||
|
|
||
|
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. nit: I would define the reserved uninitialized value here as a public value:
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, called it |
||
| private: | ||
| // Default initialization results in an invalid ID. | ||
| ReprTy Val = std::numeric_limits<ReprTy>::max(); | ||
tylanphear marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| explicit CallbackID(ReprTy Val) : Val{Val} {} | ||
|
|
||
| public: | ||
| CallbackID() = default; | ||
| friend class Context; | ||
| friend struct DenseMapInfo<CallbackID>; | ||
| }; | ||
|
|
||
| protected: | ||
| LLVMContext &LLVMCtx; | ||
|
|
@@ -83,7 +99,7 @@ class Context { | |
| /// A counter used for assigning callback IDs during registration. The same | ||
| /// counter is used for all kinds of callbacks so we can detect mismatched | ||
| /// registration/deregistration. | ||
| CallbackID NextCallbackID = 0; | ||
| CallbackID::ReprTy NextCallbackID = 0; | ||
|
|
||
| /// Remove \p V from the maps and returns the unique_ptr. | ||
| std::unique_ptr<Value> detachLLVMValue(llvm::Value *V); | ||
|
|
@@ -263,6 +279,27 @@ class Context { | |
| // TODO: Add callbacks for instructions inserted/removed if needed. | ||
| }; | ||
|
|
||
| } // namespace llvm::sandboxir | ||
| } // namespace sandboxir | ||
|
|
||
| // DenseMap info for CallbackIDs | ||
| template <> struct DenseMapInfo<sandboxir::Context::CallbackID> { | ||
| using CallbackID = sandboxir::Context::CallbackID; | ||
| using ReprInfo = DenseMapInfo<CallbackID::ReprTy>; | ||
|
|
||
| static CallbackID getEmptyKey() { | ||
| return CallbackID{ReprInfo::getEmptyKey()}; | ||
| } | ||
| static CallbackID getTombstoneKey() { | ||
| return CallbackID{ReprInfo::getTombstoneKey()}; | ||
| } | ||
| static unsigned getHashValue(const CallbackID &ID) { | ||
| return ReprInfo::getHashValue(ID.Val); | ||
| } | ||
| static bool isEqual(const CallbackID &LHS, const CallbackID &RHS) { | ||
| return ReprInfo::isEqual(LHS.Val, RHS.Val); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace llvm | ||
|
|
||
| #endif // LLVM_SANDBOXIR_CONTEXT_H | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -686,7 +686,7 @@ void Context::runMoveInstrCallbacks(Instruction *I, const BBIterator &WhereIt) { | |
| Context::CallbackID Context::registerEraseInstrCallback(EraseInstrCallback CB) { | ||
| assert(EraseInstrCallbacks.size() <= MaxRegisteredCallbacks && | ||
| "EraseInstrCallbacks size limit exceeded"); | ||
| CallbackID ID = NextCallbackID++; | ||
| CallbackID ID{NextCallbackID++}; | ||
|
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. We should probably have a check here that
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. Hmm it's probably better to put this check in the constructor.
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. Makes sense; put an |
||
| EraseInstrCallbacks[ID] = CB; | ||
| return ID; | ||
| } | ||
|
|
@@ -700,7 +700,7 @@ Context::CallbackID | |
| Context::registerCreateInstrCallback(CreateInstrCallback CB) { | ||
| assert(CreateInstrCallbacks.size() <= MaxRegisteredCallbacks && | ||
| "CreateInstrCallbacks size limit exceeded"); | ||
| CallbackID ID = NextCallbackID++; | ||
| CallbackID ID{NextCallbackID++}; | ||
| CreateInstrCallbacks[ID] = CB; | ||
| return ID; | ||
| } | ||
|
|
@@ -713,7 +713,7 @@ void Context::unregisterCreateInstrCallback(CallbackID ID) { | |
| Context::CallbackID Context::registerMoveInstrCallback(MoveInstrCallback CB) { | ||
| assert(MoveInstrCallbacks.size() <= MaxRegisteredCallbacks && | ||
| "MoveInstrCallbacks size limit exceeded"); | ||
| CallbackID ID = NextCallbackID++; | ||
| CallbackID ID{NextCallbackID++}; | ||
| MoveInstrCallbacks[ID] = CB; | ||
| return ID; | ||
| } | ||
|
|
||
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.
What does "Repr" stand for in
ReprTy?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.
Representation, as
uint64_tis the underlying representation. Open to suggestion on the naming here.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.
Wdyt about
ValTy?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.
Seems reasonable to me.