Page MenuHomePhabricator

[PATCH][ARM] Enable the use of SVC anywhere in an IT block
ClosedPublic

Authored by avieira on Sep 1 2017, 2:28 AM.

Details

Summary

Hi,

This patch fixes the assembler to allow assembling a SVC instruction anywhere in an IT block and not only as the last instruction of said IT block.

Cheers,
Andre

Diff Detail

Repository
rL LLVM

Event Timeline

avieira created this revision.Sep 1 2017, 2:28 AM
olista01 accepted this revision.Sep 1 2017, 3:13 AM
olista01 added a subscriber: olista01.

LGTM.

This looks like it should also apply to HVC and SMC, but they have different IT block behaviour (not predicable and only allowed as last instruction, respectively), which we already model correctly.

This revision is now accepted and ready to land.Sep 1 2017, 3:13 AM

Will be worth uploading the diff again with context git diff -U999999.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
8725 ↗(On Diff #113518)

My guess is that this is here because tSVC sets isCall but in the Arm ARM it is classified as an exception, and isn't marked as "not permitted in IT block" or "Outside or last in IT block". Exceptions are permitted to return to an IT block as long as they preserve the condition codes.

I've a couple of thoughts; the first would be to make this clearer by adding a comment along the lines of isCall is true for SVC, but it is not an IT block terminator. The other would be to move the test into the if statement, for example (MCID.isCall() && Inst.getOpcode() == ARM::tSVC).

The other thought I've not got a good answer for, does tSVC really need to set isCall given that an SVC is an exception rather than a call and must be implemented as if it were an exception (save all registers, stack, condition codes etc.).

avieira updated this revision to Diff 113528.Sep 1 2017, 3:26 AM

Added bigger diff.

avieira updated this revision to Diff 113531.Sep 1 2017, 3:48 AM

I moved the check into the existing isCall check and updated the comments.

As for checking whether isCall makes sense for SVC, I think that should be done separately? I don't know exactly what other implications that may have.

rengolin edited edge metadata.Sep 1 2017, 3:51 AM

Folks, please refrain from approving your own patches minutes after posting. This is not how open source is done. If you need review, let people review your patches. Peter's comment is a clear example on why this is important.

As a rule of thumb, we recommend waiting a few days before pinging the patch if there are no reviews and after a few pings, please contact the code owners directly, first via the review itself (using @), but also by email or IRC.

If nothing works, raise the issue in the dev list.

I'm ok with the proposed changes. I agree the SVC setting isCall is a much wider question than just this patch and shouldn't block it, however it would be worth having a think about though. As Renato suggests it would be good to wait for a couple of working days to see there are any comments from later timezones before committing.

OK, I will wait a few days to see if there are any objections.

This revision was automatically updated to reflect the committed changes.