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

[Renato Golin via llvm-commits]

rengolin added subscribers: jroelofs, compnerd.
rengolin added a comment.

Hi Peter,

Overall, I think this is a good idea. It makes a pass unnecessary, generate the expected code directly and reduces the size considerably.

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'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'm still trying to understand the difference between the old and the new, but the new code looks ok so far. I'll let you know as I progress.

Thanks!
--renato


================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2943
@@ +2942,3 @@
+  bool UseGOT_PREL =
+      !(GV->hasHiddenVisibility() || GV->isStrongDefinitionForLinker());
+
----------------
Can you elaborate on 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(
----------------
Is it 8 for thumb1, too?

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


http://reviews.llvm.org/D13650