[PATCH] D22760: [ARM] Implement -mimplicit-it assembler option

[Oliver Stannard via llvm-commits]

olista01 marked 4 inline comments as done.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:176
@@ -149,3 +175,3 @@
     ARMCC::CondCodes Cond;    // Condition for IT block.
     unsigned Mask:4;          // Condition mask for instructions.
                               // Starting at first 1 (from lsb).
----------------
t.p.northover wrote:
> I think this is probably trying too hard to match the encoding, especially as it has to be mangled anyway. I'd suggest a simple left-to-right bitfield (so ITT -> 0b1, ITTT ->0b11, ITTE -> 0b10).
> 
> I think that would make quite a few places easier to understand:
> 
>   * ParseInstruction wouldn't have to deal with quite so much weirdness
>   * processInstruction could call getITEncoding (which would be about as simple as it is now).
>   * rewind becomes just a right shift.
>   * extend becomes a left shift + or.
This encoding has the advantage of also encoding the length of the block. In your scheme, IT and ITE would have the same encoding, so we would have to record the block length in the parser as another operand, so this wouldn't really simplify things.

Using getITMaskEncoding in processInstruction simplifies that function (and avoids repeating the xoring code), so all of the complexity is now contained within the functions operating on ITState.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8985
@@ +8984,3 @@
+  case ARM::t2LDR_PRE:
+  case ARM::t2LDR_POST:
+  case ARM::tMOVr:
----------------
t.p.northover wrote:
> I think you're missing some here. Grepping for "[^r]GPR[^n]" in ARMInstrThumb2.td suggests at least t2LDRB_PRE and friends.
> 
> I think it would be more robust to iterate through the operands looking for PC (plus checks for a few special instructions). It ought to be possible to use the MCInstrDesc to determine whether there's a write to PC</vigorous hand-waving>.
t2LDRB_PRE doesn't need to be in this list, as it is UNPREDICTABLE when Rn is PC. Unfortunately, this is one of the many UNPREDICTABLE instructions we don't currently diagnose.

However, I agree that it will be more robust to check the MCInstrDesc instead of having a hard-coded list of instructions.

There is MCInstrDesc::mayAffectControlFlow, which does almost what we want, but unfortunately can't be completely accurate without some instruction-specific knowledge, which I've had to keep in this function.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9033
@@ +9032,3 @@
+    }
+    rewindImplicitITPosition();
+  }
----------------
t.p.northover wrote:
> I'm not really convinced we need to rewind at all. Currently, we do the following (in both places it gets called):
> 
>   * Extend with a tautologous ITState.Cond
>   * Hack in the real condition on success.
>   * rewind on failure.
> 
> What can't we just extend with the correct condition when we know it's sensible?
The problem here is that instruction matching changes depending on the IT state. There are checks in checkTargetMatchPredicate (called from MatchInstructionImpl), which cause the correct Thumb2 arithmetic instructions to be selected, depending on the IT state. If we didn't extend the IT block before matching, this would select the instructions that are valid outside an IT block, which we couldn't then extend the IT block to cover without changing their flag-setting behaviour.

However, instruction matching depends only on the presence of an IT block, not the condition of it, so we can hack in the condition later, and don't have to re-run matching with the two (when extending an existing block) or 15 (when creating a new block) possible conditions.


Repository:
  rL LLVM

https://reviews.llvm.org/D22760