[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#1284997, @MatzeB wrote:

> In https://reviews.llvm.org/D49671#1283704, @materi wrote:
>
> > I don't like any implementation that has first-class and second-class MachineOperands. At least I think it's a bad idea to have this in the default implementation, doing it in a target hook makes sense though.
>
>
> I don't think having a target specific hook is good enough here because some of the problematic operands are generated by generic register allocation code; you will get them as soon as you have subregisters in the mix.


Hmm, also if you use TracksSubRegLiveness?

> 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 also wonder if the only purpose of implicit operands added in regalloc is for liveness modeling? What about things like AH/AL on x86 where writing a subregister spills over to the neighbor?

In https://reviews.llvm.org/D49671#1284998, @MatzeB wrote:

> In https://reviews.llvm.org/D49671#1282086, @materi wrote:
>
> > This patch is causing some problems in my out-of-tree back-end. We add some MachineOperands on the fly for some uses/defs that are conditional or depend on some circumstances, like how registers were allocated, or which depth a loop is at in a loop nest. With this patch, these manually added operands don't work as we intend.
>
>
> Would you be in a position to mark your extra operands as explicit operands since the machine does appear to be reading/writing them in your case? (Maybe you have to mark the instruction as variadic, or I would be open to invent a new MCInstrDesc flag if that helps...)


Yes I think we could change them to explicit operands. I have not tried, but I can't think of any reason it would not work.

>> I'm wondering if we could maybe keep the old flexible way to look at MachineOperands and put the functionality which sets the latency to zero in the getOperandLatency hook instead?
>> 
>> I also think it's a bad idea to have some MachineOperands be special but it's not visible in the debug printouts. Now we have to read the tablegen file to understand if an operand is considered fake or not.
> 
> I don't like it either, but this fact is true independently of this patch...

As I understand it, once the operand is attached to the MI, LLVM treated it the same regardless where it came from. So the semantics of the operand was obvious from looking at MI->dump().


https://reviews.llvm.org/D49671