This is an archive of the discontinued LLVM Phabricator instance.

Thumb2SizeReduction: Check the correct set of registers for LDMIA.
ClosedPublic

Authored by pcc on May 4 2015, 12:40 PM.

Details

Summary

The register set for LDMIA begins at offset 3, not 4. We were previously
missing the short encoding of this instruction in the case where the base
register was the first register in the register set.

Also clean up some dead code:

  • The isARMLowRegister check is redundant with what VerifyLowRegs does.
  • Remove handling of LDMDB instruction, which has no short encoding (and does not appear in ReduceTable).

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 24906.May 4 2015, 12:40 PM
pcc retitled this revision from to Thumb2SizeReduction: Check the correct set of registers for LDMIA..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: rengolin.
pcc added a subscriber: Unknown Object (MLST).
rengolin edited edge metadata.May 5 2015, 11:17 AM

Hi Peter,

I can't see a call to VerifyLowRegs in ReduceLoadStore, how can you guarantee that it was indeed checked?

Otherwise, looks ok.

cheers,
--renato

pcc added a comment.May 5 2015, 11:23 AM

The call is in Thumb2SizeReduce::ReduceSpecial (see line 566 in new code) before the call to ReduceLoadStore.

rengolin accepted this revision.May 5 2015, 11:38 AM
rengolin edited edge metadata.
In D9485#166124, @pcc wrote:

The call is in Thumb2SizeReduce::ReduceSpecial (see line 566 in new code) before the call to ReduceLoadStore.

Ah, yes, I thought so. What you could do at least is to replace the test into an assert, so that we can catch the odd cases in the future in the case someone decide to call the same function without validating first. And also for documentation. :)

With the assert, LGTM. Thanks!

This revision is now accepted and ready to land.May 5 2015, 11:38 AM
pcc added a comment.May 5 2015, 12:33 PM

Ah, yes, I thought so. What you could do at least is to replace the test into an assert, so that we can catch the odd cases in the future in the case someone decide to call the same function without validating first. And also for documentation. :)

Done. I found that the assert was firing in cases where we were loading from SP, so I also needed to remove the t2LDMIA instruction from the set of isPCOk instructions in VerifyLowRegs. This seems fine, as the non-writeback version of this instruction only accepts r0-r7 in the register list.

pcc updated this revision to Diff 24962.May 5 2015, 12:33 PM
pcc edited edge metadata.

Add assert

Perfect, Thanks! LGTM.

This revision was automatically updated to reflect the committed changes.