Page MenuHomePhabricator

[MachineInstruction] Put implicit operands part of MCID before ones that aren't in addOperand()
AbandonedPublic

Authored by jonpa on May 30 2018, 2:37 AM.

Details

Reviewers
atrick
MatzeB
Summary

This addresses the same problem as https://reviews.llvm.org/D46837. Instead of fixing findDefIdx(), this problem can be handled by making addOperand() insert an added operand in a more sensible way.

The reason that this is needed is that a target may lazily change opcode on an instrution with setDesc(), in order to keep all the operands, and then add one extra register operand. If this new register operand is part of the MCInstrDesc (and therefore mapped to a latency in the InstrRW SchedWrites list), it must be inserted in the right place. Therefore, it should be inserted before any operands not part of the MCInstrDesc, but yet found in the MachineInstr operands list. This can typically be an impl-def of a super register.

This is not a guarantee that all operands part of MCInstrDesc end up in the right place, but merely an improvement to let targets do this trick when it's sure it will work out if looking at just the MCInstrDesc and the InstrRW for the instruction.

There are also tests that now fail, which I suspect is due to debug output differences. Before fixing those, we should decide if this patch is the right way to handle this problem.

Not sure if this is reasonable, or if it should instead be demanded by target to handle this on its own (i.e. rebuild the instruction from scratch). This seems however to work well on SystemZ at the moment.

This could possibly be guarded to only be done if the target has a SchedModel.

Diff Detail

Event Timeline

jonpa created this revision.May 30 2018, 2:37 AM

I would consider setDesc() a rare/exceptional operation. So I'm not sure I like an extra loop for the common addOperand() maybe we can rather provide an insertOperand() to accomodate the setDesc() scenario you describe?

or maybe prependImplicitDef()?

jonpa abandoned this revision.Jul 21 2018, 4:12 AM

This was actually fixed entirely for SystemZ, by just rewriting one setDesc() call to instead rebuilding the new opcode from scratch. So, this is actually not an issue any more of my concern, so I will abandon this for now. Thanks for taking a look, though!