[SandboxIR] Implement UndefValue #107628
[SandboxIR] Implement UndefValue #107628
Conversation
This patch implements sandboxir::UndefValue mirroring llvm::UndefValue.
| /// For isa/dyn_cast. | ||
| static bool classof(const sandboxir::Value *From) { | ||
| return From->getSubclassID() == ClassID::UndefValue || | ||
| From->getSubclassID() == ClassID::PoisonValue; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You modelled it correctly. Poison inherits from Undef, but an undef value/instance should not be poison.
There was a problem hiding this comment.
static bool classof(const sandboxir::Value *From) {
return From->getSubclassID() == ClassID::UndefValue;
}
should work.
There was a problem hiding this comment.
Yes, isa<> works because it takes care of upcasting. But classof() would be wrong, it should return true if From is a PoisonValue.
There was a problem hiding this comment.
I believe the current classof UndefValue is wrong. It should only test for ClassId::UndefValue.
isa<PoisonValue>(Undef)
should/must be false.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
isa(Undef) should/must be false.
We are already testing that: EXPECT_FALSE(isa<sandboxir::PoisonValue>(Val: Undef)); // Undef is not Poison
There was a problem hiding this comment.
LGTM,
isa<PoisonValue>(Undef_val) is false
| @@ -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 | |||
There was a problem hiding this comment.
Hmm what do yo mean? This just checks that a Poison is also an Undef.
|
Surprise, I misread the LLVM documentation. |
|
Please ignore me: llvm-project/llvm/include/llvm/IR/Constants.h Line 1433 in de88d7d |
| /// For isa/dyn_cast. | ||
| static bool classof(const sandboxir::Value *From) { | ||
| return From->getSubclassID() == ClassID::UndefValue || | ||
| From->getSubclassID() == ClassID::PoisonValue; |
There was a problem hiding this comment.
LGTM,
isa<PoisonValue>(Undef_val) is false
| @@ -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 | |||
This patch implements sandboxir::UndefValue mirroring llvm::UndefValue.