[PATCH] [Thumb1] Re-write emitThumbRegPlusImmediate

[Oliver Stannard]

================
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
----------------
t.p.northover wrote:
> 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.
I think that the assertion on line 230 (second version of the patch) should catch this case, but I'll add one closer to here to be sure.

http://reviews.llvm.org/D6211