Glow: Unneeded Splat when lowering ConcatNode

[et-nivard]

A Splat instruction is being added to initialize dest Tensor when lowering Splat node (https://github.com/pytorch/glow/blob/master/lib/IR/IRGen.cpp#L272). It seems unneeded because dest Tensor gets eventually written by subsequent InsertTensor instructions. Can we remove this Splat?

[nickgg]

Pretty sure in this case we can replace this Splat with an AllocActivation, have you tried it?

[jfix71]

Yeah think we can just remove the splat, have you tried doing so and running all tests in debug?

[et-nivard]

Yes, I tried but few tests using Concat node fail because of assertion https://github.com/pytorch/glow/blob/master/lib/IR/IR.cpp#L297. "@in and @inout operands should be initialized before their first use". Dest operand of InsertTensor is @inout so we don't meet this requirement when removing Splat initialization. One of the tests failing is caffe2.concatAddAxis from Caffe2ImporterTest.
I guess we won't be able to remove this apparently unneeded Splat unless we can refine this assertion within the sequence of InsertTensors lowered from the Concat node.

[jfix71]

Yeah, I thought I remembered there being an issue for doing this 🙁. IMO a preferable alternative to relaxing the assertion on a particular special case is to be able to differentiate between two different cases which are currently both OperandKind::InOut -- one where we partially overwrite an operand (i.e. InsertTensor's Dest), and one where we reuse an operand, reading in some values and updating them in place (i.e. QuantizationProfile's Histogram). I wonder if we could introduce something like a OperandKind::PartialOut for this first case, which would allow us to not worry about liveness here.

[nickgg]

That seems semi dangerous, equivalent to just not checking InOut since I think we’ll eventually change all InOuts to PartialOuts.

An alternative could be to add a new Instruction which explicitly doesn’t initialize an activation (but marks it as initialized). That way we can use it only when doing a conversion that doesn’t need initialization.

Not sure if this is a related thought but InOut and inPlace fields overlap in meaning a bit as well.

[jfix71]

Hm, not sure I see it as dangerous per se. It'd be more accurate of what's actually going on with that operand. We'd likely change most InOut to PartialOut, but that would provide more accurate liveness analysis. Adding a new Instruction that marks an activation as initialized also works. Or a bool member to AllocActivation that marks it as not needing initialization (though this may be more error prone/dangerous than PartialOut.

[nickgg]

Feels about the same as PartialOut yeah. Don't feel too strongly about this, just seems like we're choosing between explicit and implicit and would lean towards explicit.

[et-nivard]

I agree. Both approaches should work. Could we decide then which one to implement? I guess explicit approach should be quicker to implement.

[nickgg]

Agree, it's on my list to look at but if you get time to do it earlier, feel free.