Skip to content

[SandboxIR] Implement UnaryInstruction class #101541

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 1 commit into from
Aug 2, 2024
Merged

[SandboxIR] Implement UnaryInstruction class #101541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 27 additions & 7 deletions llvm/include/llvm/SandboxIR/SandboxIR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@
// |
// +- InsertElementInst
// |
// +- LoadInst
// |
// +- OpaqueInst
// |
// +- PHINode
Expand All @@ -78,6 +76,10 @@
// |
// +- StoreInst
// |
// +- UnaryInstruction -+- LoadInst
// | |
// | +- CastInst
// |
// +- UnaryOperator
//
// Use
Expand Down Expand Up @@ -108,6 +110,7 @@ class Function;
class Instruction;
class SelectInst;
class BranchInst;
class UnaryInstruction;
class LoadInst;
class ReturnInst;
class StoreInst;
Expand Down Expand Up @@ -836,10 +839,26 @@ class BranchInst : public Instruction {
#endif
};

class LoadInst final : public Instruction {
/// An abstract class, parent of unary instructions.
class UnaryInstruction : public Instruction {
protected:
UnaryInstruction(ClassID ID, Opcode Opc, llvm::Instruction *LLVMI,
Context &Ctx)
: Instruction(ID, Opc, LLVMI, Ctx) {}

public:
static bool classof(const Instruction *I) {
return isa<LoadInst>(I) || isa<CastInst>(I);
}
static bool classof(const Value *V) {
return isa<Instruction>(V) && classof(cast<Instruction>(V));
Copy link

@tschuett tschuett Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

isa + cast again?

if (Instruction *I = dyn_cast<Instruction>(V))
  return classof(I);
return 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.

Isn't this a bit too verbose? Three lines instead of one. And btw this is exactly how it's implemented in InstrTypes.h.

Copy link

Choose a reason for hiding this comment

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

We had this discussion before. The manual asks for dyn_cast.

Copy link
Contributor Author

@vporpo vporpo Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you may be misinterpreting the manual. This pattern is very common throughout LLVM. It can't be that no one noticed that all this code does not conform to the coding style. include/llvm/IR/ in particular contains this exact pattern 119 times across a couple of header files.

Copy link

Choose a reason for hiding this comment

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

Widely use is a bad argument.
"Note that you should not use an isa<> test followed by a cast<>, for that use the dyn_cast<> operator."

Copy link
Contributor

Choose a reason for hiding this comment

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

although I like the less redundant dyn_cast version, there is merit to keeping code short. I would bring this up on discourse if you feel strongly about this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Widely use is a bad argument.

True, but if it's widely used then it probably means that either:
(i) we are not conflicting with what the manual says and we are misinterpreting it, or
(ii) using isa<> + cast<> in this particular case is preferable despite what the manual says, because it is much less verbose.

In any case, I don't think we should be spending too much time arguing about this, it's just a style issue and both isa<> + cast<> and dyn_cast<> versions exist in the LLVM codebase.

Copy link

Choose a reason for hiding this comment

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

Probably, it was a common pattern to use isa + cast. Then the manual was updated: please use dyn_cast, but not all occurrences of isa +cast were converted to dyn_cast. That is why you find this pattern in many places.

I contribute to:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
We try to model LLVM-IR, but we are forced to deviations. There is also some freedom, i.e., ExtOrTrunc.
My point is : we use switches.

}
};

class LoadInst final : public UnaryInstruction {
/// Use LoadInst::create() instead of calling the constructor.
LoadInst(llvm::LoadInst *LI, Context &Ctx)
: Instruction(ClassID::Load, Opcode::Load, LI, Ctx) {}
: UnaryInstruction(ClassID::Load, Opcode::Load, LI, Ctx) {}
friend Context; // for LoadInst()
Use getOperandUseInternal(unsigned OpIdx, bool Verify) const final {
return getOperandUseDefault(OpIdx, Verify);
Expand Down Expand Up @@ -1371,7 +1390,7 @@ class GetElementPtrInst final : public Instruction {
#endif
};

class CastInst : public Instruction {
class CastInst : public UnaryInstruction {
static Opcode getCastOpcode(llvm::Instruction::CastOps CastOp) {
switch (CastOp) {
case llvm::Instruction::ZExt:
Expand Down Expand Up @@ -1408,8 +1427,9 @@ class CastInst : public Instruction {
/// Use Context::createCastInst(). Don't call the
/// constructor directly.
CastInst(llvm::CastInst *CI, Context &Ctx)
: Instruction(ClassID::Cast, getCastOpcode(CI->getOpcode()), CI, Ctx) {}
friend Context; // for SBCastInstruction()
: UnaryInstruction(ClassID::Cast, getCastOpcode(CI->getOpcode()), CI,
Ctx) {}
friend Context; // for SBCastInstruction()
friend class PtrToInt; // For constructor.
Use getOperandUseInternal(unsigned OpIdx, bool Verify) const final {
return getOperandUseDefault(OpIdx, Verify);
Expand Down
53 changes: 40 additions & 13 deletions llvm/unittests/SandboxIR/SandboxIRTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,7 @@ define void @foo(ptr %arg0, ptr %arg1) {
auto *BB = &*F->begin();
auto It = BB->begin();
auto *Ld = cast<sandboxir::LoadInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(Ld));
auto *VLd = cast<sandboxir::LoadInst>(&*It++);
auto *Ret = cast<sandboxir::ReturnInst>(&*It++);

Expand Down Expand Up @@ -1489,79 +1490,105 @@ define void @foo(i32 %arg, float %farg, double %darg, ptr %ptr) {

// Check classof(), getOpcode(), getSrcTy(), getDstTy()
auto *ZExt = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::ZExtInst>(ZExt));
auto *ZExtI = cast<sandboxir::ZExtInst>(ZExt);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(ZExtI));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(ZExtI));
EXPECT_EQ(ZExt->getOpcode(), sandboxir::Instruction::Opcode::ZExt);
EXPECT_EQ(ZExt->getSrcTy(), Ti32);
EXPECT_EQ(ZExt->getDestTy(), Ti64);

auto *SExt = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::SExtInst>(SExt));
auto *SExtI = cast<sandboxir::SExtInst>(SExt);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(SExt));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(SExtI));
EXPECT_EQ(SExt->getOpcode(), sandboxir::Instruction::Opcode::SExt);
EXPECT_EQ(SExt->getSrcTy(), Ti32);
EXPECT_EQ(SExt->getDestTy(), Ti64);

auto *FPToUI = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::FPToUIInst>(FPToUI));
auto *FPToUII = cast<sandboxir::FPToUIInst>(FPToUI);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPToUI));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPToUII));
EXPECT_EQ(FPToUI->getOpcode(), sandboxir::Instruction::Opcode::FPToUI);
EXPECT_EQ(FPToUI->getSrcTy(), Tfloat);
EXPECT_EQ(FPToUI->getDestTy(), Ti32);

auto *FPToSI = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::FPToSIInst>(FPToSI));
auto *FPToSII = cast<sandboxir::FPToSIInst>(FPToSI);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPToSI));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPToSII));
EXPECT_EQ(FPToSI->getOpcode(), sandboxir::Instruction::Opcode::FPToSI);
EXPECT_EQ(FPToSI->getSrcTy(), Tfloat);
EXPECT_EQ(FPToSI->getDestTy(), Ti32);

auto *FPExt = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::FPExtInst>(FPExt));
auto *FPExtI = cast<sandboxir::FPExtInst>(FPExt);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPExt));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPExtI));
EXPECT_EQ(FPExt->getOpcode(), sandboxir::Instruction::Opcode::FPExt);
EXPECT_EQ(FPExt->getSrcTy(), Tfloat);
EXPECT_EQ(FPExt->getDestTy(), Tdouble);

auto *PtrToInt = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::PtrToIntInst>(PtrToInt));
auto *PtrToIntI = cast<sandboxir::PtrToIntInst>(PtrToInt);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(PtrToInt));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(PtrToIntI));
EXPECT_EQ(PtrToInt->getOpcode(), sandboxir::Instruction::Opcode::PtrToInt);
EXPECT_EQ(PtrToInt->getSrcTy(), Tptr);
EXPECT_EQ(PtrToInt->getDestTy(), Ti32);

auto *IntToPtr = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::IntToPtrInst>(IntToPtr));
auto *IntToPtrI = cast<sandboxir::IntToPtrInst>(IntToPtr);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(IntToPtr));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(IntToPtrI));
EXPECT_EQ(IntToPtr->getOpcode(), sandboxir::Instruction::Opcode::IntToPtr);
EXPECT_EQ(IntToPtr->getSrcTy(), Ti32);
EXPECT_EQ(IntToPtr->getDestTy(), Tptr);

auto *SIToFP = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::SIToFPInst>(SIToFP));
auto *SIToFPI = cast<sandboxir::SIToFPInst>(SIToFP);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(SIToFP));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(SIToFPI));
EXPECT_EQ(SIToFP->getOpcode(), sandboxir::Instruction::Opcode::SIToFP);
EXPECT_EQ(SIToFP->getSrcTy(), Ti32);
EXPECT_EQ(SIToFP->getDestTy(), Tfloat);

auto *UIToFP = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::UIToFPInst>(UIToFP));
auto *UIToFPI = cast<sandboxir::UIToFPInst>(UIToFP);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(UIToFP));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(UIToFPI));
EXPECT_EQ(UIToFP->getOpcode(), sandboxir::Instruction::Opcode::UIToFP);
EXPECT_EQ(UIToFP->getSrcTy(), Ti32);
EXPECT_EQ(UIToFP->getDestTy(), Tfloat);

auto *Trunc = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::TruncInst>(Trunc));
auto *TruncI = cast<sandboxir::TruncInst>(Trunc);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(Trunc));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(TruncI));
EXPECT_EQ(Trunc->getOpcode(), sandboxir::Instruction::Opcode::Trunc);
EXPECT_EQ(Trunc->getSrcTy(), Ti32);
EXPECT_EQ(Trunc->getDestTy(), Ti16);

auto *FPTrunc = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::FPTruncInst>(FPTrunc));
auto *FPTruncI = cast<sandboxir::FPTruncInst>(FPTrunc);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPTrunc));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(FPTruncI));
EXPECT_EQ(FPTrunc->getOpcode(), sandboxir::Instruction::Opcode::FPTrunc);
EXPECT_EQ(FPTrunc->getSrcTy(), Tdouble);
EXPECT_EQ(FPTrunc->getDestTy(), Tfloat);

auto *BitCast = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::BitCastInst>(BitCast));
auto *BitCastI = cast<sandboxir::BitCastInst>(BitCast);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(BitCast));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(BitCastI));
EXPECT_EQ(BitCast->getOpcode(), sandboxir::Instruction::Opcode::BitCast);
EXPECT_EQ(BitCast->getSrcTy(), Ti32);
EXPECT_EQ(BitCast->getDestTy(), Tfloat);

auto *AddrSpaceCast = cast<sandboxir::CastInst>(&*It++);
EXPECT_TRUE(isa<sandboxir::AddrSpaceCastInst>(AddrSpaceCast));
auto *AddrSpaceCastI = cast<sandboxir::AddrSpaceCastInst>(AddrSpaceCast);
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(AddrSpaceCast));
EXPECT_TRUE(isa<sandboxir::UnaryInstruction>(AddrSpaceCastI));
EXPECT_EQ(AddrSpaceCast->getOpcode(),
sandboxir::Instruction::Opcode::AddrSpaceCast);
EXPECT_EQ(AddrSpaceCast->getSrcTy(), Tptr);
Expand Down
Loading