HomePhabricator

[ARM][MC] Move information about variadic register defs into tablegen
Audit RequiredrL348114

Description

[ARM][MC] Move information about variadic register defs into tablegen

Currently, variadic operands on an MCInst are assumed to be uses,
because they come after the defs. However, this is not always the case,
for example the Arm/Thumb LDM instructions write to a variable number of
registers.

This adds a property of instruction definitions which can be used to
mark variadic operands as defs. This only affects MCInst, because
MachineInstruction already tracks use/def per operand in each instance
of the instruction, so can already represent this.

This property can then be checked in MCInstrDesc, allowing us to remove
some special cases in ARMAsmParser::isITBlockTerminator.

Differential revision: https://reviews.llvm.org/D54853

Details

Event Timeline

This commit now requires audit.Dec 3 2018, 2:36 AM
reames added a subscriber: reames.Dec 10 2018, 1:06 PM

This looks like a generally good improvement, but is it really the case that if any variable operand is a def that all must be? It would seem more general to have a list of variable operands defs and a list of variable operand uses. I have an alternate use case which would require that at some point, but I'm wondering about the current ARM backend use case at the current time.

In the ARM backend, we only use variable_ops for the LDM and STM instructions, where the variadic operands are either all uses or all defs. I'm less familiar with the other backends, but all of the other cases where variable_ops is used look similar.

If a target does need both variable uses and defs on the same instruction, I assume that we'd need to add a way to represent that to the MCInst or MCOperand, as the sequence of uses and defs would vary between different instances of the instruction.simillar

In the ARM backend, we only use variable_ops for the LDM and STM instructions, where the variadic operands are either all uses or all defs. I'm less familiar with the other backends, but all of the other cases where variable_ops is used look similar.

If a target does need both variable uses and defs on the same instruction, I assume that we'd need to add a way to represent that to the MCInst or MCOperand, as the sequence of uses and defs would vary between different instances of the instruction.simillar

Thanks Oliver,

I plan to use your new flag in llvm-mca to mark variable operands that are known to be register definitions.

Your design works quite well for ARM and other upstream targets. I verified that your new flag is sufficient to describe variadic opcodes defined by upstream targets behave similarly. Overall I see this as an improvement.

However, I also share the same concers as Philip: the current design is specifically tailored for ARM, and it may not be generic enough to support targets that specify a mix of def/use variable operands for their opcodes. That being said, for now it is not a problem (i.e. I hope this design is not going to change anytime soon).

Speaking about your change:
Your patch only marks a few of variadic opcodes with flag variadicOpsAreDefs. For example, I don't see flag variadicOpsAreDefs specified for the following instructions:

FLDMXIA/FLDMXIA_UPD/VLDMIA/VLDMIA_UPD.

Shouldn't your flag be applied to those opcodes too?

Thanks
-Andrea