This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Avoid using ARM instructions in Thumb mode
ClosedPublic

Authored by samparker on Jan 30 2017, 7:53 AM.

Details

Summary

The Requires class overrides the target requirements of an instruction, rather than adding to them, so all ARM instructions need to include the IsARM predicate when they have overwitten requirements. This caused the swp and swpb instructions to be allowed in thumb mode assembly, and the ARM encoding of CDP to be selected in codegen (which is different for conditional instructions).

Diff Detail

Event Timeline

samparker created this revision.Jan 30 2017, 7:53 AM

Small comment about tests, but otherwise, looks good.

I'm slightly worried that SWP is deprecated in v6 and v7 and even optional in v7VE.

Not for this patch, but I'm wondering if we could make the instruction itself v4/v5 and have an InstAlias for v6/v7 so that we only generate is for old arches but still supports assembly with it.

Makes sense?
--renato

test/CodeGen/Thumb2/conditional-cdp.ll
1 ↗(On Diff #86286)

It seems we're also missing ldc/stc/mcr etc. in the Thumb2 dir.

Hi Renato,

Yes, I will look into the v7VE issue; however, I'm not sure how using an InstAlias would be possible since the operation of SWP and LDREX/STREX pair seems different to my understanding. The STREX returns a store success flag while SWP can atomically swap the value between a register and a memory location. Am I misunderstanding the use of these instructions?

Cheers,
Sam

Yes, I will look into the v7VE issue;

Just to be clear, this can surely be for another patch. For this patch, just the additional tests on Thumb2 should do it.

however, I'm not sure how using an InstAlias would be possible since the operation of SWP and LDREX/STREX pair seems different to my understanding. The STREX returns a store success flag while SWP can atomically swap the value between a register and a memory location. Am I misunderstanding the use of these instructions?

They are different in syntax, yes, but being optional means we can't generate SWPs for v7VE. We already generate LDREX/STREX pairs, so I think it should be fine, and this change won't make it more likely to be generated in any way, so this is not impacting this patch.

This is just something to have in mind to do. Maybe a FIXME comment in the code would help.

--renato

Ok, will do. Just having a root around the tests, the missing Thumb2 tests appear to be in intrinsics-coprocessor.ll in the ARM folder instead.

cheers,
sam

Ok, will do. Just having a root around the tests, the missing Thumb2 tests appear to be in intrinsics-coprocessor.ll in the ARM folder instead.

Right. I'd either move the CDP test in there or duplicate all tests in the Thumb2 dir. Doesn't matter which way.

cheers,
--renato

samparker updated this revision to Diff 86414.Jan 31 2017, 5:49 AM

Moved the Thumb2 tests into their own file, incorporating the new conditional cdp instruction. Added a fixme comment for SWP/SWPB.

rengolin accepted this revision.Jan 31 2017, 6:07 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jan 31 2017, 6:07 AM
This revision was automatically updated to reflect the committed changes.