[PATCH] D13650: ARM/ELF: Better codegen for global variable addresses.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

> However, I'm worried about the impact of this on Thumb1 code. It seems all tests are assuming Thumb2, and Thumb1's restrictions for literal pools are a lot more restrictive. I'm adding a few people that used to care about Thumb1. Feel free to add more.


I believe that `test/CodeGen/ARM/load-global.ll` is a Thumb1 test case (it targets `thumbv6-linux-gnueabi`).

While investigating this I found that fast ISel isn't even being used for Thumb{1,2} on ELF targets. There's a flag `-arm-force-fast-isel` to force it on, and if I do so I find that generated code is badly broken in Thumb1 mode (e.g. it uses Thumb2 encodings). I've modified the `fast-isel-pic.ll` test to force fast ISel for Thumb2, which seems to be correct for loads at least.

> I'm also concerned with potential edge cases, so if you could run the the test-suite on Thumb2 and ARM, that would be great.


I'll see if I can get access to a machine to do this on, the only ones I have access to at the moment are Android devices.


================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2943
@@ +2942,3 @@
+  bool UseGOT_PREL =
+      !(GV->hasHiddenVisibility() || GV->isStrongDefinitionForLinker());
+
----------------
rengolin wrote:
> Can you elaborate on this change?
I've modified the commit message to call out this change.

================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2947
@@ +2946,3 @@
+  unsigned ARMPCLabelIndex = AFI->createPICLabelUId();
+  unsigned PCAdj = isThumb2 ? 4 : 8;
+  ARMConstantPoolValue *CPV = ARMConstantPoolConstant::Create(
----------------
rengolin wrote:
> Is it 8 for thumb1, too?
It is, done (but see main reply).

================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2958
@@ +2957,3 @@
+  unsigned TempReg = MF->getRegInfo().createVirtualRegister(&ARM::rGPRRegClass);
+  unsigned Opc = Subtarget->isThumb() ? ARM::t2LDRpci : ARM::LDRcp;
+  MachineInstrBuilder MIB =
----------------
rengolin wrote:
> Inconsistent isThumb with t2 instruction. Also, would be good to get all checks against a consistent ISA. I believe this can be used for all three.
I've done what I think is the right thing for t1 once it gets fixed properly.


http://reviews.llvm.org/D13650