Skip to content

[SandboxIR] Implement UndefValue #107628

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.

Sign up for GitHub

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 2 commits into from
Sep 9, 2024
Merged

[SandboxIR] Implement UndefValue #107628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
197dad4
[SandboxIR] Implement UndefValue
vporpo Aug 21, 2024
245533f
fixup! [SandboxIR] Implement UndefValue
vporpo Sep 6, 2024
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
61 changes: 55 additions & 6 deletions llvm/include/llvm/SandboxIR/SandboxIR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ class Value {
friend class ConstantStruct; // For `Val`.
friend class ConstantAggregateZero; // For `Val`.
friend class ConstantPointerNull; // For `Val`.
friend class UndefValue; // For `Val`.
friend class PoisonValue; // For `Val`.

/// All values point to the context.
Expand Down Expand Up @@ -1020,10 +1021,61 @@ class ConstantPointerNull final : public Constant {
#endif
};

// TODO: Inherit from UndefValue.
class PoisonValue final : public Constant {
// TODO: Inherit from ConstantData.
class UndefValue : public Constant {
protected:
UndefValue(llvm::UndefValue *C, Context &Ctx)
: Constant(ClassID::UndefValue, C, Ctx) {}
UndefValue(ClassID ID, llvm::Constant *C, Context &Ctx)
: Constant(ID, C, Ctx) {}
friend class Context; // For constructor.

public:
/// Static factory methods - Return an 'undef' object of the specified type.
static UndefValue *get(Type *T);

/// If this Undef has array or vector type, return a undef with the right
/// element type.
UndefValue *getSequentialElement() const;

/// If this undef has struct type, return a undef with the right element type
/// for the specified element.
UndefValue *getStructElement(unsigned Elt) const;

/// Return an undef of the right value for the specified GEP index if we can,
/// otherwise return null (e.g. if C is a ConstantExpr).
UndefValue *getElementValue(Constant *C) const;

/// Return an undef of the right value for the specified GEP index.
UndefValue *getElementValue(unsigned Idx) const;

/// Return the number of elements in the array, vector, or struct.
unsigned getNumElements() const {
return cast<llvm::UndefValue>(Val)->getNumElements();
}

/// For isa/dyn_cast.
static bool classof(const sandboxir::Value *From) {
return From->getSubclassID() == ClassID::UndefValue ||
From->getSubclassID() == ClassID::PoisonValue;
Copy link

@tschuett tschuett Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

isa<PoisonValue>(Undef) is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm did you mean return From->getSubclassID() == ClassID::UndefValue || isa<PoisonValue(From) ?
I would not prefer this because because it hides the fact that PoisonValue corresponds to just a single ClassID::PoisonValue.

Hmm or did you mean checking the llvm-side with something like isa<PoisonValue>(From->Val) ?

The reason why I don't like this is that it looks like it is slightly slower: Because you need to get From->Val and then run classof() which does the comparison ID == UndefValue || ID == PoisonValue.

Copy link

Choose a reason for hiding this comment

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

You modelled it correctly. Poison inherits from Undef, but an undef value/instance should not be poison.

Copy link

Choose a reason for hiding this comment

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

 static bool classof(const sandboxir::Value *From) {
    return From->getSubclassID() == ClassID::UndefValue;
}

should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, isa<> works because it takes care of upcasting. But classof() would be wrong, it should return true if From is a PoisonValue.

Copy link

Choose a reason for hiding this comment

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

I believe the current classof UndefValue is wrong. It should only test for ClassId::UndefValue.

isa<PoisonValue>(Undef)

should/must be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

classof() should return true when From is either the current class type (that is UndefValue) or its sub-classes (that is PoisonValue). This is implemented the same way as in llvm/IR/Constants.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isa(Undef) should/must be false.

We are already testing that: EXPECT_FALSE(isa<sandboxir::PoisonValue>(Val: Undef)); // Undef is not Poison

Copy link
Member

Choose a reason for hiding this comment

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

LGTM,

isa<PoisonValue>(Undef_val) is false

}
unsigned getUseOperandNo(const Use &Use) const final {
llvm_unreachable("UndefValue has no operands!");
}
#ifndef NDEBUG
void verify() const override {
assert(isa<llvm::UndefValue>(Val) && "Expected an UndefValue!");
}
void dumpOS(raw_ostream &OS) const override {
dumpCommonPrefix(OS);
dumpCommonSuffix(OS);
}
#endif
};

class PoisonValue final : public UndefValue {
PoisonValue(llvm::PoisonValue *C, Context &Ctx)
: Constant(ClassID::PoisonValue, C, Ctx) {}
: UndefValue(ClassID::PoisonValue, C, Ctx) {}
friend class Context; // For constructor.

public:
Expand All @@ -1049,9 +1101,6 @@ class PoisonValue final : public Constant {
static bool classof(const sandboxir::Value *From) {
return From->getSubclassID() == ClassID::PoisonValue;
}
unsigned getUseOperandNo(const Use &Use) const final {
llvm_unreachable("PoisonValue has no operands!");
}
#ifndef NDEBUG
void verify() const override {
assert(isa<llvm::PoisonValue>(Val) && "Expected a PoisonValue!");
Expand Down
1 change: 1 addition & 0 deletions llvm/include/llvm/SandboxIR/SandboxIRValues.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ DEF_CONST(ConstantStruct, ConstantStruct)
DEF_CONST(ConstantVector, ConstantVector)
DEF_CONST(ConstantAggregateZero, ConstantAggregateZero)
DEF_CONST(ConstantPointerNull, ConstantPointerNull)
DEF_CONST(UndefValue, UndefValue)
DEF_CONST(PoisonValue, PoisonValue)

#ifndef DEF_INSTR
Expand Down
30 changes: 30 additions & 0 deletions llvm/lib/SandboxIR/SandboxIR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2437,6 +2437,32 @@ PointerType *ConstantPointerNull::getType() const {
Ctx.getType(cast<llvm::ConstantPointerNull>(Val)->getType()));
}

UndefValue *UndefValue::get(Type *T) {
auto *LLVMC = llvm::UndefValue::get(T->LLVMTy);
return cast<UndefValue>(T->getContext().getOrCreateConstant(LLVMC));
}

UndefValue *UndefValue::getSequentialElement() const {
return cast<UndefValue>(Ctx.getOrCreateConstant(
cast<llvm::UndefValue>(Val)->getSequentialElement()));
}

UndefValue *UndefValue::getStructElement(unsigned Elt) const {
return cast<UndefValue>(Ctx.getOrCreateConstant(
cast<llvm::UndefValue>(Val)->getStructElement(Elt)));
}

UndefValue *UndefValue::getElementValue(Constant *C) const {
return cast<UndefValue>(
Ctx.getOrCreateConstant(cast<llvm::UndefValue>(Val)->getElementValue(
cast<llvm::Constant>(C->Val))));
}

UndefValue *UndefValue::getElementValue(unsigned Idx) const {
return cast<UndefValue>(Ctx.getOrCreateConstant(
cast<llvm::UndefValue>(Val)->getElementValue(Idx)));
}

PoisonValue *PoisonValue::get(Type *T) {
auto *LLVMC = llvm::PoisonValue::get(T->LLVMTy);
return cast<PoisonValue>(T->getContext().getOrCreateConstant(LLVMC));
Expand Down Expand Up @@ -2580,6 +2606,10 @@ Value *Context::getOrCreateValueInternal(llvm::Value *LLVMV, llvm::User *U) {
It->second = std::unique_ptr<PoisonValue>(
new PoisonValue(cast<llvm::PoisonValue>(C), *this));
return It->second.get();
case llvm::Value::UndefValueVal:
It->second = std::unique_ptr<UndefValue>(
new UndefValue(cast<llvm::UndefValue>(C), *this));
return It->second.get();
case llvm::Value::ConstantArrayVal:
It->second = std::unique_ptr<ConstantArray>(
new ConstantArray(cast<llvm::ConstantArray>(C), *this));
Expand Down
59 changes: 59 additions & 0 deletions llvm/unittests/SandboxIR/SandboxIRTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,7 @@ define void @foo() {
// Check classof() and creation.
auto *Poison = cast<sandboxir::PoisonValue>(I0->getOperand(0));
EXPECT_EQ(Poison->getType(), Int32Ty);
EXPECT_TRUE(isa<sandboxir::UndefValue>(Poison)); // Poison is Undef
Copy link

Choose a reason for hiding this comment

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

same problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm what do yo mean? This just checks that a Poison is also an Undef.

Copy link

Choose a reason for hiding this comment

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

This is correct.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM, Poison value is an undef.

// Check get().
auto *NewPoison = sandboxir::PoisonValue::get(Int32Ty);
EXPECT_EQ(NewPoison, Poison);
Expand Down Expand Up @@ -670,6 +671,64 @@ define void @foo() {
sandboxir::PoisonValue::get(Int8Ty));
}

TEST_F(SandboxIRTest, UndefValue) {
parseIR(C, R"IR(
define void @foo() {
%i0 = add i32 undef, undef
%i1 = add <2 x i32> undef, undef
%i2 = extractvalue {i32, i8} undef, 0
ret void
}
)IR");
Function &LLVMF = *M->getFunction("foo");
sandboxir::Context Ctx(C);

auto &F = *Ctx.createFunction(&LLVMF);
auto &BB = *F.begin();
auto It = BB.begin();
auto *I0 = &*It++;
auto *I1 = &*It++;
auto *I2 = &*It++;
auto *Int32Ty = sandboxir::Type::getInt32Ty(Ctx);
auto *Int8Ty = sandboxir::Type::getInt8Ty(Ctx);
auto *Zero32 = sandboxir::ConstantInt::get(Int32Ty, 0u);
auto *One32 = sandboxir::ConstantInt::get(Int32Ty, 1u);

// Check classof() and creation.
auto *Undef = cast<sandboxir::UndefValue>(I0->getOperand(0));
EXPECT_EQ(Undef->getType(), Int32Ty);
EXPECT_FALSE(isa<sandboxir::PoisonValue>(Undef)); // Undef is not Poison
// Check get().
auto *NewUndef = sandboxir::UndefValue::get(Int32Ty);
EXPECT_EQ(NewUndef, Undef);
auto *NewUndef2 =
sandboxir::UndefValue::get(sandboxir::PointerType::get(Ctx, 0u));
EXPECT_NE(NewUndef2, Undef);
// Check getSequentialElement().
auto *UndefVector = cast<sandboxir::UndefValue>(I1->getOperand(0));
auto *SeqElm = UndefVector->getSequentialElement();
EXPECT_EQ(SeqElm->getType(), Int32Ty);
// Check getStructElement().
auto *UndefStruct = cast<sandboxir::UndefValue>(I2->getOperand(0));
auto *StrElm0 = UndefStruct->getStructElement(0);
auto *StrElm1 = UndefStruct->getStructElement(1);
EXPECT_EQ(StrElm0->getType(), Int32Ty);
EXPECT_EQ(StrElm1->getType(), Int8Ty);
// Check getElementValue(Constant)
EXPECT_EQ(UndefStruct->getElementValue(Zero32),
sandboxir::UndefValue::get(Int32Ty));
EXPECT_EQ(UndefStruct->getElementValue(One32),
sandboxir::UndefValue::get(Int8Ty));
// Check getElementValue(unsigned)
EXPECT_EQ(UndefStruct->getElementValue(0u),
sandboxir::UndefValue::get(Int32Ty));
EXPECT_EQ(UndefStruct->getElementValue(1u),
sandboxir::UndefValue::get(Int8Ty));
// Check getNumElements().
EXPECT_EQ(UndefVector->getNumElements(), 2u);
EXPECT_EQ(UndefStruct->getNumElements(), 2u);
}

TEST_F(SandboxIRTest, Use) {
parseIR(C, R"IR(
define i32 @foo(i32 %v0, i32 %v1) {
Expand Down
Loading