[SandboxIR] Implement UnaryInstruction class #101541
Conversation
vporpo
requested review from
slackito,
echristo,
alinas,
tschuett,
aeubanks,
tmsri and
Sterling-Augustine
| return I->getOpcode() == Instruction::Opcode::Load || isa<CastInst>(I); | ||
| } | ||
| static bool classof(const Value *V) { | ||
| return isa<Instruction>(V) && classof(cast<Instruction>(V)); |
There was a problem hiding this comment.
isa + cast again?
if (Instruction *I = dyn_cast<Instruction>(V))
return classof(I);
return false;
There was a problem hiding this comment.
Isn't this a bit too verbose? Three lines instead of one. And btw this is exactly how it's implemented in InstrTypes.h.
There was a problem hiding this comment.
We had this discussion before. The manual asks for dyn_cast.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Yeah it's not super useful, but it is another way to check if an instruction has a single operand. I don't feel too strongly about keeping though. On the other hand it's not a huge burden to add it in either. There are just a handful of instructions that inherit from it. |
|
It makes SandboxIR closer to LLVM-IR. |
Having multiple ways of checking if an instruction has a single operand is worse than just having one way, and checking if an instruction has exactly one operand doesn't seem super useful/generic. Any code we can avoid adding is good.
Sure but it's really just used as an internal implementation detail to share some allocation code. There are some places throughout LLVM that check for UnaryInstruction, but there are better ways to do what they want to do. |
|
in general I'm a big advocate of only adding what you actually need and use; you can always add missing functionality later if it comes up |
But this is very much the llvm-way of checking things, which is by casting to the appropriate class with isa<>, and dyn_cast<>.
I agree with this, but in this case there the I think that we should not try to diverge from LLVM IR unless there is some technical issue or something, which I don't feel this is the case. In the docs we are claiming that Sandbox IR feels-like LLVM, so we should try to stick to this as much as possible. |
|
|
||
| public: | ||
| static bool classof(const Instruction *I) { | ||
| return I->getOpcode() == Instruction::Opcode::Load || isa<CastInst>(I); |
There was a problem hiding this comment.
weird that the load and cast check use different methods (opcode vs isa)
There was a problem hiding this comment.
Indeed. There is no CastInst opcode, so we need isa<CastInst>(I). The alternative is a large switch case with the opcodes of the sub-classes of CastInst.
I guess we could use isa<LoadInst>(I) || isa<CastInst>(I).
This patch implements sandboxir::UnaryInstruction class and updates sandboxir::LoadInst and sandboxir::CastInst to inherit from it instead of sandboxir::Instruction.
This patch implements sandboxir::UnaryInstruction class and updates sandboxir::LoadInst and sandboxir::CastInst to inherit from it instead of sandboxir::Instruction.