Page MenuHomePhabricator

[ARM][SchedModels] Get rid of IsLdrAm2ScaledPred
ClosedPublic

Authored by evgeny777 on Oct 23 2020, 4:21 AM.

Diff Detail

Event Timeline

evgeny777 created this revision.Oct 23 2020, 4:21 AM
evgeny777 requested review of this revision.Oct 23 2020, 4:21 AM

Could you please add more context to the diff? I don't remember how IsLdstsoScaledPredX2 was defined.

evgeny777 updated this revision to Diff 300300.Oct 23 2020, 8:08 AM

Uploaded diff with full context

Should it always have been checking operand 4 then? I think that makes sense

(More of a commit message might make it more obvious why changes are being made ;) )

Should it always have been checking operand 4 then? I think that makes sense

(More of a commit message might make it more obvious why changes are being made ;) )

I was about to ask something similar...
The original predicate is checking operand 3, not 4. I am a bit confused.

Should it always have been checking operand 4 then? I think that makes sense

IMO, it was incorrectly checking for operand 3. I've got an assertion when switched to MC pred

dmgreen accepted this revision.Oct 23 2020, 8:50 AM

LGTM then. Cheers

This revision is now accepted and ready to land.Oct 23 2020, 8:50 AM

Should it always have been checking operand 4 then? I think that makes sense

IMO, it was incorrectly checking for operand 3. I've got an assertion when switched to MC pred

(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...

(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.

(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).

(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...

@andreadb Well, it's interesting thought and I, honestly, do not know. However with LDRBT it doesn't seem to be the case. Here is handwritten MIR:

name:            test_ldrbt
tracksRegLiveness: true
body: |
  bb.0:
    liveins: $r1, $r3, $r4
    %1:gprnopc = COPY $r1
    %3:gprnopc = COPY $r3
    %4:gprnopc = COPY $r4
    %3:gprnopc, %4:gprnopc = LDRBT_POST_REG %4:gprnopc, %1:gprnopc, 20492, 0, $noreg

    $r0 = COPY %3:gprnopc
    BX_RET 14, $noreg, implicit $r0

If you're interested you can try transforming it to asm with llc -start-before=greedy -filetype=asm ... and check operand sequence in assembly with llvm-mc --debug

So, it doesn't look like the lowering is doing anything odd with the operand sequence. It should be fine.
At this point, the only explanation is that the original predicate was never really executed for that write variant. So it was never tested. I don't see other alternatives honestly.

andreadb accepted this revision.Oct 23 2020, 10:25 AM

I'm happy with that explanation, and would be happy with this patch going ahead. The mir test above seems to show it failing before and working now.

I'm happy with that explanation, and would be happy with this patch going ahead. The mir test above seems to show it failing before and working now.

Exactly:

llc: /home/andrea/llvm-project/llvm/include/llvm/CodeGen/MachineOperand.h:536: int64_t llvm::MachineOperand::getImm() const: Assertion `isImm() && "Wrong MachineOperand accessor"' failed.

It looks like that predicate has always been wrong.
Sorry for causing panic :-P (I guess, better safe than sorry).

This revision was automatically updated to reflect the committed changes.