[PATCH] D22760: [ARM] Implement -mimplicit-it assembler option
Tim Northover via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 25 11:03:48 PDT 2016
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
More information about the llvm-commits
mailing list