This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Be Conservative when coalescing into SP
AbandonedPublic

Authored by zzheng on Jun 5 2015, 3:23 PM.

Details

Summary
r226200 allows coalescing into SP but missed a corner case:
    
  %vreg907<def> = COPY %SP; GPRnopc:%vreg907
  %vreg908<def> = t2SUBri %vreg907, 80, pred:14, pred:%noreg, opt:%noreg; GPRnopc:%vreg908,%vreg907
  %SP<def> = COPY %vreg908; GPRnopc:%vreg908
    
When %vreg908 is coalesced into SP but %vreg907 is not, it's likely to emit
an illegal instruction:
  sub sp, r12, #80
    
In Thumb2, we can only use SP as Rd if and only if Rn is also SP. This patch,
when trying remove "%SP<def> = COPY %vreg908", checks if %vreg907 is SP
already. If not, be conservative and do not coalesce %vreg908 into SP.

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng updated this revision to Diff 27238.Jun 5 2015, 3:23 PM
zzheng retitled this revision from to [ARM] Be Conservative when coalescing into SP.
zzheng updated this object.
zzheng edited the test plan for this revision. (Show Details)
zzheng added reviewers: hfinkel, apazos, mcrosier.
zzheng set the repository for this revision to rL LLVM.
zzheng edited edge metadata.Jun 5 2015, 3:24 PM
zzheng added a subscriber: Unknown Object (MLST).
qcolombet edited edge metadata.Jun 5 2015, 3:38 PM

Hi,

The proposed target hook is not flexible enough IMHO.
What if the reserved register is a source register?
What if the destination MI has several destination register, which one are we looking at?

I believe the right answer would be: provide the ReservedReg and the operand index.

Thanks,
-Quentin

PS: Please upload the context with your patch. This helps reviewing.

MatzeB added a comment.Jun 5 2015, 3:38 PM

Hmm I think the RegisterCoalescer is correct. Maybe it would be better
to use a register class for the Rn input that doesn't have sp in it and create new opcodes for the "sub sp" cases.

Hi Matthias,

I tend to agree with you that the coalescer is correct since the encoding permit it. On the other hand, this is the only pass I am aware of that messed up with the reserved register. E.g., the machine copy propagation pass would not create such pattern.

Cheers,
-Quentin

MatzeB added a comment.Jun 5 2015, 3:44 PM

I just looked at the ARM instruction definitions and it seems that the T2I_bin_ii12rs multiclass used for SUB has a no-SP restriction (it is using the rGPR class instead of GPRnopc) but on the Rm input instead of the Rn input, maybe this is simply a specification typo?

Hi,

This looks more like a bug with how we model t2SUBri's register usage to me. For example we still allow someone to assemble "sub r12, sp, #80" with this change. Also, the kind of logic you're trying to add here seems to be completely incidental to whether the register is reserved or not: either the register classes model the constraints of the instruction, or they don't.

I'd probably split up the definitions to create something like Thumb's "tSUBspi" and make the normal variants take and define an rGPR.

Also, there should be a test for this.

Cheers.

Tim.

qcolombet requested changes to this revision.Jun 5 2015, 3:52 PM
qcolombet edited edge metadata.

SGTM.

Please fix the encoding of the SUB instruction!

Q.

This revision now requires changes to proceed.Jun 5 2015, 3:52 PM
mcrosier resigned from this revision.Aug 5 2015, 11:18 AM
mcrosier removed a reviewer: mcrosier.
zzheng abandoned this revision.Mar 16 2017, 11:10 AM