[PATCH] D49671: [SchedModel] Propagate read advance cycles to implicit operands outside instruction descriptor

[mattias.v.eriksson@ericsson.com via Phabricator via llvm-commits]

materi added a comment.

In https://reviews.llvm.org/D49671#1285281, @materi wrote:

> In https://reviews.llvm.org/D49671#1284997, @MatzeB wrote:
>
> > Some words about the different kinds of operands:
> >
> > The extra operands do make sense semantically and are necessary for our modeling of things. The thing I regret though is that just being an implicit operand can mean two things today: It's an operand that isn't explicitly emitted to assembly/encoded or it's an operand that does not correspond to a read/write access in hardware or both. In this patch we only want to catch the 2nd kind, but not purely cases of the first. While it is unfortunate to not have this modeled as two separate bits today, it feels to me like the heuristic is close enough. Should we try again with an extra `MO.isImplicit()` in the condition?
>
>
> I agree that this overloading is unfortunate! How hard would it be to split these operands in two different types?


I tried adding a bit in MachineOperand which is set by VirtRegMap when handling SuperDefs/SuperKills/SuperDeads. That makes it possible to filter out that kind of dependency in addPhysRegDeps. It seems to work fine and avoids having to look at MCInstrDesc.

Adding a bit in MachineOperand increases the size of the class; I don't know if that's a big deal.

In https://reviews.llvm.org/D49671#1294302, @bjope wrote:

> For the record, I haven't put too much thought into the above. But afaict we can have IMPLICIT_DEF instructions after RA already today. Maybe the problem is that RA passes are adding implicit defs to real instructions, when they actually should add IMPLICIT_DEF instructions instead when modelling undef?


I think this is an interesting idea. But does this work without enabling TracksSubRegLiveness if LivenessAfterRegalloc is enabled? (This is so complicated! Maybe it's not an issue at all?)

Anyway, I think that the current code is broken and may miscompile code with SDNPVariadic instructions.


https://reviews.llvm.org/D49671