Skip to content

[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

Merged
merged 5 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 47 additions & 7 deletions llvm/include/llvm/SandboxIR/Context.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@

#include <cstdint>

namespace llvm::sandboxir {
namespace llvm {
namespace sandboxir {

class Argument;
class BBIterator;
Expand All @@ -37,10 +38,28 @@ 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 ValTy = uint64_t;
static constexpr const ValTy InvalidVal = 0;

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

static constexpr const ReprTy UninitVal = std::numeric_limits<ReprTy>::max();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, called it InvalidVal.

private:
// Default initialization results in an invalid ID.
ValTy Val = InvalidVal;
explicit CallbackID(ValTy Val) : Val{Val} {
assert(Val != InvalidVal && "newly-created ID is invalid!");
}

public:
CallbackID() = default;
friend class Context;
friend struct DenseMapInfo<CallbackID>;
};

protected:
LLVMContext &LLVMCtx;
Expand Down Expand Up @@ -83,7 +102,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::ValTy NextCallbackID = 1;

/// Remove \p V from the maps and returns the unique_ptr.
std::unique_ptr<Value> detachLLVMValue(llvm::Value *V);
Expand Down Expand Up @@ -263,6 +282,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::ValTy>;

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
6 changes: 3 additions & 3 deletions llvm/lib/SandboxIR/Context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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++};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have a check here that NextCallbackID is not equal to the reserved value, with something like:

assert(NextCallbackID != CallbackID::UninitVal && "Matches the reserved value!");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it's probably better to put this check in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense; put an assert in the constructor.

EraseInstrCallbacks[ID] = CB;
return ID;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down
Loading