[PATCH] D90024: [ARM][SchedModels] Get rid of IsLdrAm2ScaledPred

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In D90024#2350536 <https://reviews.llvm.org/D90024#2350536>, @dmgreen wrote:

>> (paranoia level).
>> Are we sure that the lowering of MachineInstr to MCInst is preserving the operand sequence? Can it be that the immediate is at position 4 for the MCInst only?
>> I have no idea how an ldrbt looks like as a MachineInstr. The original check should have triggered an assertion too for MachineInstr then...
>
> I was not aware that was a thing that could happen!
>
> I'm unsure if anything would ever generate an ldrbt with negative postinc reg and shift from codegen, so it might be difficult to test. And may never have been tested in the past.

I  guess the answer is in `llvm::LowerARMMachineInstrToMCInst`.

On x86 for example the lowering of the operands is always "trivial" (meaning that the ordering of the machine operands in the MachineInstr is still preserved in the final MCInst). It is basically a foreach loop that produces an MCOperand for every MachineOperand. No operands change in position nor are added/removed. Otherwise, it is unsafe to use MCInstrDesc to describe MCInst too (MCInstDesc is designed for MachineInstr).

I suggest to double-check that it is fine for this particular case. It may well be that the ldrbt is a pseudo, and that the rule was never really triggered in practice.
Otherwise it is not safe to use a MCSchedPredicate for this patch unless we provide a way to map machine operand indices to valid MCOperand indices (only for those instructions for which lowering is not trivial).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90024/new/

https://reviews.llvm.org/D90024