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

[Tim Northover via llvm-commits]

t.p.northover added a subscriber: t.p.northover.

================
Comment at: include/llvm/MC/MCParser/MCTargetAsmParser.h:223
@@ -220,1 +222,3 @@
+  /// output streamer, if the target does not emit them immediately.
+  virtual void FlushPendingInstructions(MCStreamer &Out) { }
 };
----------------
Lower case 'f' would be better.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:59
@@ +58,3 @@
+static cl::opt<ImplicitItModeTy> ImplicitItMode(
+    "mimplicit-it", cl::init(ImplicitItModeTy::ARMOnly),
+    cl::desc("Allow conditional instructions outdside of an IT block"),
----------------
The 'm' is more an artefact of the GCC compatible driver naming conventions. In LLVM it should probably be "arm-implicit-it".

================
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).
----------------
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.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8985
@@ +8984,3 @@
+  case ARM::t2LDR_PRE:
+  case ARM::t2LDR_POST:
+  case ARM::tMOVr:
----------------
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>.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9033
@@ +9032,3 @@
+    }
+    rewindImplicitITPosition();
+  }
----------------
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?

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9062-9065
@@ +9061,6 @@
+
+  // Try to match in a new IT block. The matcher doesn't check the actual
+  // condition, so we create an IT block with a dummy condition, and fix it up
+  // once we know the actual condition.
+  extendImplicitITBlock(ARMCC::AL);
+  if (MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm) ==
----------------
The condition argument actually seems to be entirely unused (except for an assertion). I'd split this function into `startImplicitITBlock` and `extendImplicitITBlock`, neither of which take a condition. Especially given its implementation is completely split anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D22760