ARM: Only enforce 4-byte alignment on Thumb-2 functions with constant pools.

[pcc]

Summary

This appears to have been introduced back in r76698 as part of an unrelated
change. I can find no official ARM documentation stating that Thumb-2 functions
require 4-byte alignment; in fact, ARM documentation appears to contradict
this (see, e.g., ARM Architecture Reference Manual Thumb-2 Supplement,
section 2.6.1: "Thumb-2 enforces 16-bit alignment on all instructions.").

Also remove code that sets alignment for ARM functions, which is redundant
with code in the MachineFunction constructor, and remove the hidden
-arm-align-constant-islands flag, which has been enabled by default since
r146739 (Dec 2011) and has probably received sufficient testing by now.

[pcc]

• Fix test

[rengolin]

lib/Target/ARM/ARMConstantIslandPass.cpp

416 ↗

(On Diff #24095)

This is not just about standard Thumb alignment, but Thumb functions with constant pools, which are also expected to align 4, if memory serves me right. Is that what you're testing down there on your new test?

I wonder if that's going to open an old corner case...

[pcc]

lib/Target/ARM/ARMConstantIslandPass.cpp

416 ↗

(On Diff #24095)

This is not just about standard Thumb alignment, but Thumb functions with constant pools, which are also expected to align 4, if memory serves me right.

Exactly. We are now only setting the alignment in cases where a constant pool makes it necessary (see lines 504-508 of the new file). I've retitled the change to make this clear.

Is that what you're testing down there on your new test?

Yes.

[rengolin]

Great! LGTM, Thanks!

llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp

 STATISTIC(NumJTMoved,    "Number of jump table destination blocks moved");
 STATISTIC(NumJTInserted, "Number of jump table intermediate blocks inserted");
 
 
 static cl::opt<bool>
 AdjustJumpTableBlocks("arm-adjust-jump-tables", cl::Hidden, cl::init(true),
           cl::desc("Adjust basic block layout to better use TB[BH]"));
 
-// FIXME: This option should be removed once it has received sufficient testing.
-static cl::opt<bool>
-AlignConstantIslands("arm-align-constant-islands", cl::Hidden, cl::init(true),
-          cl::desc("Align constant islands in code"));
-
 /// UnknownPadding - Return the worst case padding that could result from
 /// unknown offset bits.  This does not include alignment padding caused by
 /// known offset bits.
 ///
 /// @param LogAlign log2(alignment)
 /// @param KnownBits Number of known low offset bits.
 static inline unsigned UnknownPadding(unsigned LogAlign, unsigned KnownBits) {
   if (KnownBits < LogAlign)
if (isThumb2 && AdjustJumpTableBlocks) {
     scanFunctionJumpTables();
     MadeChange |= reorderThumb2JumpTables();
     // Data is out of date, so clear it. It'll be re-computed later.
     T2JumpTables.clear();
     // Blocks may have shifted around. Keep the numbering up to date.
     MF->RenumberBlocks();
   }
 
-  // Thumb1 functions containing constant pools get 4-byte alignment.
-  // This is so we can keep exact track of where the alignment padding goes.
-
-  // ARM and Thumb2 functions need to be 4-byte aligned.
-  if (!isThumb1)
-    MF->ensureAlignment(2);  // 2 = log2(4)
-
   // Perform the initial placement of the constant pool entries.  To start with,
   // we put them all at the end of the function.
   std::vector<MachineInstr*> CPEMIs;
   if (!MCP->isEmpty())
     doInitialPlacement(CPEMIs);
 
   /// The next UID to take is the first unused one.
   AFI->initPICLabelUId(CPEMIs.size());
ARMConstantIslands::doInitialPlacement(std::vector<MachineInstr*> &CPEMIs) {
   // Create the basic block to hold the CPE's.
   MachineBasicBlock *BB = MF->CreateMachineBasicBlock();
   MF->push_back(BB);
 
   // MachineConstantPool measures alignment in bytes. We measure in log2(bytes).
   unsigned MaxAlign = Log2_32(MCP->getConstantPoolAlignment());
 
   // Mark the basic block as required by the const-pool.
-  // If AlignConstantIslands isn't set, use 4-byte alignment for everything.
-  BB->setAlignment(AlignConstantIslands ? MaxAlign : 2);
+  BB->setAlignment(MaxAlign);
 
   // The function needs to be as aligned as the basic blocks. The linker may
   // move functions around based on their alignment.
   MF->ensureAlignment(BB->getAlignment());
 
   // Order the entries in BB by descending alignment.  That ensures correct
   // alignment of all entries as long as BB is sufficiently aligned.  Keep
   // track of the insertion point for each alignment.  We are going to bucket
*ARMConstantIslands::findConstPoolEntry(unsigned CPI,
   return nullptr;
 }
 
 /// getCPELogAlign - Returns the required alignment of the constant pool entry
 /// represented by CPEMI.  Alignment is measured in log2(bytes) units.
 unsigned ARMConstantIslands::getCPELogAlign(const MachineInstr *CPEMI) {
   assert(CPEMI && CPEMI->getOpcode() == ARM::CONSTPOOL_ENTRY);
 
-  // Everything is 4-byte aligned unless AlignConstantIslands is set.
-  if (!AlignConstantIslands)
-    return 2;
-
   unsigned CPI = CPEMI->getOperand(1).getIndex();
   assert(CPI < MCP->getConstants().size() && "Invalid constant pool index.");
   unsigned Align = MCP->getConstants()[CPI].getAlignment();
   assert(isPowerOf2_32(Align) && "Invalid CPE alignment");
   return Log2_32(Align);
 }
 
 /// scanFunctionJumpTables - Do a scan of the function, building up

llvm/trunk/test/CodeGen/ARM/thumb-alignment.ll

+; RUN: llc -relocation-model=pic -mtriple=thumbv7-unknown-linux -o - %s | FileCheck %s
+
+@x = external global i32
+
+; CHECK: .globl foo
+; CHECK-NEXT: .align  2
+define i32* @foo() {
+  ret i32* @x
+}
+
+; CHECK: .globl bar
+; CHECK-NEXT: .align  1
+define i32* @bar() {
+  ret i32* zeroinitializer
+}

llvm/trunk/test/MC/ARM/elf-thumbfunc-reloc.ll

entry:
   call void @foo()
   ret void
 }
 
 
 ; make sure that bl 0 <foo> (fff7feff) is correctly encoded
 ; CHECK: Sections [
 ; CHECK:   SectionData (
-; CHECK:     0000: 704700BF 2DE90048 FFF7FEFF BDE80088
+; CHECK:     0000: 70472DE9 0048FFF7 FEFFBDE8 0088
 ; CHECK:   )
 ; CHECK: ]
 
 ; CHECK:      Relocations [
 ; CHECK-NEXT:   Section {{.*}} .rel.text {
-; CHECK-NEXT:     0x8 R_ARM_THM_CALL foo 0x0
+; CHECK-NEXT:     0x6 R_ARM_THM_CALL foo 0x0
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {{.*}} .rel.ARM.exidx {
 ; CHECK-NEXT:     0x0 R_ARM_PREL31 .text 0x0
 ; CHECK-NEXT:     0x8 R_ARM_PREL31 .text 0x0
 ; CHECK-NEXT:   }
 ; CHECK-NEXT: ]
 
 ; make sure foo is thumb function: bit 0 = 1
 ; CHECK:      Symbols [
 ; CHECK:        Symbol {
 ; CHECK:          Name: foo
 ; CHECK-NEXT:     Value: 0x1