[SandboxIR] Add InsertValueInst #106273
[SandboxIR] Add InsertValueInst #106273
Conversation
slackito
requested review from
alinas,
vporpo,
tschuett,
aeubanks,
tmsri and
Sterling-Augustine
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| BBIterator WhereIt, BasicBlock *WhereBB, Context &Ctx, | ||
| const Twine &Name = ""); | ||
|
|
||
| using idx_iterator = llvm::InsertValueInst::idx_iterator; |
There was a problem hiding this comment.
No
static bool classof(const Value *From) {
return From->getSubclassID() == ClassID::InsertValueInst;
}
There was a problem hiding this comment.
Hmm @slackito does the test pass without the classof? Shouldn't it crash here?
auto *Ins0 = cast<sandboxir::InsertValueInst>(&*It++);
There was a problem hiding this comment.
I wonder if there is a good way to check that classof has been implemented. Perhaps a test that uses the .def file and does something with <subclass>::classof() so that we get a compile-time error if we forget to implement it.
There was a problem hiding this comment.
I wrote a small test that checks EXPECT_NE(&<subclass>::classof, &Instruction::classof); and one of the insturctions is missing a classof!
There was a problem hiding this comment.
Great! Can you commit that test (and the missing classof) separately from this PR?
There was a problem hiding this comment.
Yes I will upload a PR in a few minutes.
| static Value *create(Value *Agg, Value *Val, ArrayRef<unsigned> Idxs, | ||
| BBIterator WhereIt, BasicBlock *WhereBB, Context &Ctx, | ||
| const Twine &Name = ""); | ||
|
|
There was a problem hiding this comment.
Missing getInedexedType(). If it is missing for a reason then we should add a TODO comment.
There was a problem hiding this comment.
Are you sure? I only see getIndexedType methods in GetElementPtrInst and ExtractValueInst, not in InsertValueInst.
There was a problem hiding this comment.
Oops I was looking at the ExtractValueInst :)
| BBIterator WhereIt, BasicBlock *WhereBB, Context &Ctx, | ||
| const Twine &Name = ""); | ||
|
|
||
| using idx_iterator = llvm::InsertValueInst::idx_iterator; |
There was a problem hiding this comment.
Hmm @slackito does the test pass without the classof? Shouldn't it crash here?
auto *Ins0 = cast<sandboxir::InsertValueInst>(&*It++);
| auto *Zero = sandboxir::ConstantInt::get(Type::getInt32Ty(C), 0, Ctx); | ||
| auto *ShouldBeConstant = sandboxir::InsertValueInst::create( | ||
| Ins1->getOperand(0), Zero, ArrayRef<unsigned>({0}), BB->end(), BB, Ctx); | ||
| EXPECT_TRUE(isa<sandboxir::Constant>(ShouldBeConstant)); |
There was a problem hiding this comment.
nit: Constants are unique, so we could also do something EXPECT_EQ(ShouldBeConstant, ExpectedConstant)
| EXPECT_TRUE(isa<sandboxir::Constant>(ShouldBeConstant)); | ||
|
|
||
| // idx_begin / idx_end | ||
| auto IdxIt = Ins0->idx_begin(); |
There was a problem hiding this comment.
nit: Could you check the indices with an InsertValueInst with at least 2 indices?
- Added `classof` static method. - Added test case for `insertvalue` with more than one index. - Check constant equality in the "folded constant" case
No description provided.