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

[avieira]

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

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

[peter.smith]

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

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

8725

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]

Added bigger diff.

[avieira]

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]

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.

[peter.smith]

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.

[avieira]

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

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

 template <> inline bool IsCPSRDead<MCInst>(const MCInst *Instr) {
   return true; // In an assembly source, no need to second-guess
 }
 }
 
 // Returns true if Inst is unpredictable if it is in and IT block, but is not
 // the last instruction in the block.
 bool ARMAsmParser::isITBlockTerminator(MCInst &Inst) const {
+
   const MCInstrDesc &MCID = MII.get(Inst.getOpcode());
 
-  // All branch & call instructions terminate IT blocks.
-  if (MCID.isTerminator() || MCID.isCall() || MCID.isReturn() ||
-      MCID.isBranch() || MCID.isIndirectBranch())
+  // All branch & call instructions terminate IT blocks with the exception of
+  // SVC.
+  if (MCID.isTerminator() || (MCID.isCall() && Inst.getOpcode() != ARM::tSVC) ||
+      MCID.isReturn() || MCID.isBranch() || MCID.isIndirectBranch())
     return true;
 
   // Any arithmetic instruction which writes to the PC also terminates the IT
   // block.
   for (unsigned OpIdx = 0; OpIdx < MCID.getNumDefs(); ++OpIdx) {
     MCOperand &Op = Inst.getOperand(OpIdx);
     if (Op.isReg() && Op.getReg() == ARM::PC)
       return true;

test/MC/ARM/basic-thumb2-instructions.s

 @------------------------------------------------------------------------------
 @ SVC
 @------------------------------------------------------------------------------
         svc #0
         it eq
         svceq #255
         it ne
         swine #33
+        itt eq
+        svceq #0
+        svceq #1
 
 @ CHECK: svc  #0                      @ encoding: [0x00,0xdf]
 @ CHECK: it  eq                      @ encoding: [0x08,0xbf]
 @ CHECK: svceq  #255                    @ encoding: [0xff,0xdf]
 @ CHECK: it  ne                      @ encoding: [0x18,0xbf]
 @ CHECK: svcne  #33                     @ encoding: [0x21,0xdf]
+@ CHECK: itt    eq                      @ encoding: [0x04,0xbf]
+@ CHECK: svceq  #0                      @ encoding: [0x00,0xdf]
+@ CHECK: svceq  #1                      @ encoding: [0x01,0xdf]
 
 
 @------------------------------------------------------------------------------
 @ SXTAB
 @------------------------------------------------------------------------------
         sxtab r2, r3, r4
         sxtab r4, r5, r6, ror #0
         it lt