[PATCH] [Thumb1] Re-write emitThumbRegPlusImmediate

[Tim Northover]

Hi Oliver,

Thanks for updating the patch. To explain my previous comment a bit more:

================
Comment at: lib/Target/ARM/Thumb1RegisterInfo.cpp:250-253
@@ +249,6 @@
+  unsigned RequiredCopyInstrs = CopyOpc ? 1 : 0;
+  unsigned RangeAfterCopy = (CopyRange > Bytes) ? 0 : (Bytes - CopyRange);
+  unsigned RequiredExtraInstrs;
+  if (ExtraRange)
+    RequiredExtraInstrs = RoundUpToOddAlignment(RangeAfterCopy, ExtraRange) / ExtraRange;
+  else
----------------
olista01 wrote:
> t.p.northover wrote:
> > This doesn't seem to line up with what actually gets implemented. In particular, I think we're in deep trouble if the RangeAfterCopy actually does get rounded up.
> I'm not sure I follow. This should round RequiredExtraInstrs up to the lowest integer number of instructions that could be used, and the loop then handles as much of the immediate as possible with each instruction, so these should match up. Can you give an example where this doesn't work?
I think it falls into the limited alignment handling comments

If RangeAfterCopy has been rounded up here, then that means "(Bytes - CopyRange) % ExtraRange != 0". But "Bytes - CopyRange" is precisely what we'll have to emit with the extra instructions (Copy greedily takes as many bytes as possible). This is impossible.

I don't believe the case actually happens, because when ExtraRange != 1, we only ever emit a MOV so CopyRange == 0. But that means the clause is untested and untestable, of course: another reason to remove it.

http://reviews.llvm.org/D6211