Glow: Calling Instruction::getType yields a nullptr
I just stumbled across this landmine, and I wonder if we could do some redesign of the types involved to avoid it: Instruction is a Value, and Value is a Typed, but if you actually call getType on an instruction, it's a nullptr. Something seems broken in this model. Maybe there are some Instructions that really are Typed but it doesn't seem like it holds generally.
bertmaher
>All comments
Basically just memory-related Instructions have types (Alloc/Dealloc/TensorView/Copy). If setType() isn't set on the Instruction in InstrGen then it will return a nullptr.
It's definitely misleading/scary to have a nullptr returned otherwise; I think part of the issue here is Instructions can have multiple outputs, so calling just getType() on an Instruction which may have multi-outputs isn't sufficiently specified. As a comparison -- a Node can also have multiple outputs of course, but Node does not derive from Typed; it has its own array of types to hold a type per outputs (this is one reason why we have NodeValue, to represent a specific output of a Node).
I wonder if we should remove Type from Instruction, and have Alloc/Dealloc/TensorView/Copy derive from it. Then you can always get an operand and check its Type -- as seen in https://github.com/pytorch/glow/pull/2171 all operands must be one of {AllocActivation, WeightVar, TensorView}. Slight downside here is you would have to explicitly cast to one of them before getting the type. Could alternatively add an assert when getting the type to check if it's a nullptr...
jfix71
on 15 Dec 2018
Related issues
opti-mix
路
4Comments
qcolombet
路
5Comments
mciprian13
路
4Comments
wayneshawn
路
3Comments
gcatron
路
4Comments
Most helpful comment
Basically just memory-related Instructions have types (Alloc/Dealloc/TensorView/Copy). If
setType()isn't set on the Instruction in InstrGen then it will return a nullptr.It's definitely misleading/scary to have a nullptr returned otherwise; I think part of the issue here is Instructions can have multiple outputs, so calling just
getType()on an Instruction which may have multi-outputs isn't sufficiently specified. As a comparison -- a Node can also have multiple outputs of course, but Node does not derive from Typed; it has its own array of types to hold a type per outputs (this is one reason why we haveNodeValue, to represent a specific output of aNode).I wonder if we should remove Type from Instruction, and have Alloc/Dealloc/TensorView/Copy derive from it. Then you can always get an operand and check its Type -- as seen in https://github.com/pytorch/glow/pull/2171 all operands must be one of
{AllocActivation, WeightVar, TensorView}. Slight downside here is you would have to explicitly cast to one of them before getting the type. Could alternatively add an assert when getting the type to check if it's a nullptr...