ARM: Windows on ARM Hazard recognition support

[abdulras]

Summary

IMAGE_REL_ARM_MOV32T relocations require that the movw/movt pair relocation is
not split up and reordered. We handle this in the ARM Hazard Recognizer by
tracking the previous machine instruction and the current one. If the previous
one is a mov.w of an address entity, scan through the rest of the instructions
in the function to only permit the paired mov.t. If only the low half of an
address is being loaded (i.e. mov.w without a mov.t) then the only downside of
this tracking is that we end up scanning the remainder of the function to detect
that no other instruction can be reordered around this point.

[rengolin]

Hi Saleem,

Apart from my feature flag comment, looks good. Though, I don't know the Windows ABI that well, you may wait for more specific comments.

cheers,
--renato

lib/Target/ARM/ARMHazardRecognizer.cpp

69

Maybe we should set a feature flag for this somewhere a bit lower, and mark it true on Windows triples?

[abdulras]

lib/Target/ARM/ARMHazardRecognizer.cpp

69

Might be useful ... as much as I dislike the check as it currently stands, I think that addressing that once more of the CG is in place might make it easier to come up with a nice way to describe this scenario.

[t.p.northover]

A thought has occurred to me. The implementation via the hazard feels like a hack, so...

These MOVW/MOVT pairs are going to be coming from t2MOVi32imm or similar. Might a better solution be to modify ExpandPseudoInst so that it creates a bundle of them (on Windows) instead of 2 completely separate instructions?

That might get rid of the need for the ConstantIslands and IT block change entirely.

Cheers.

Tim.

[abdulras]

Hah, that was the first approach that I took. The problem ended up being that the psuedo-expansion occurred too early, and the instruction was still split up :-(. If there is a generic way to handle all the possible cases, that would most certainly be the preferred.

[t.p.northover]

Hi Saleem,

Hah, that was the first approach that I took.  The problem ended up being that the psuedo-expansion occurred too early, and the instruction was still split up :-(.  If there is a generic way to handle all the possible cases, that would most certainly be the preferred.

Do you remember any examples of instructions in bundles being split?
Because my natural instinct is to say that would be a bug, regardless
of whether it's exercised at the moment or not.

(In the future TLSDESC support may produce similar bundles if it
follows AArch64 implementation. Passes that split bundles would be
just as much of an issue there too).

Cheers.

Tim.

[abdulras]

Yes, the Constant Pool Island injection is one case where the instruction was split up even if I bundled them. Its quite possible that I did not get everything correct. The resultant MI would look something like this:

BUNDLE
  * %R0<def> = t2MOVi16 <ga:@"global">[TF=1], pred:14, pred:%noreg
  * %R0<def,tied1> = t2MOVTi16 %R0<tied0>, <ga:@"global">[TF=2], pred:14, pred:%noreg

This would then get split up as such:

%R0<def> = t2MOVi16 <ga:@"global">[TF=1], pred:14, pred:%noreg
t2B <BB#3>, pred:14, pred:%noreg

CONSTPOOL_ENTRY 1, <cp#1>, 4

%R0<def,tied1> = t2MOVTi16 %R0<tied0>, <ga:@"global">[TF=2], pred:14, pred:%noreg

While this is perfectly fine on other targets, on Windows, this must remain bundled as the relocation is applied to the consecutive 32-bits. The result is that the relocation would corrupt the instruction stream.

Unfortunately, I do not have a test case readily available which can demonstrate this.

[abdulras]

I think everyone agrees that this is not a particularly pretty solution. Given that any case of the bundle being split up should be considered a bug, I think that there is no reason to hold up other changes on this. This addresses all but one case: the constant island pass. I've committed a change with the bundling approach with a bit more aggressive reorder prevention. The constant island handling will have to be addressed separately and we can discuss how to best handle that separately.

lib/Target/ARM/ARMHazardRecognizer.cpp

 //===-- ARMHazardRecognizer.cpp - ARM postra hazard recognizer ------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ARMHazardRecognizer.h"
 #include "ARMBaseInstrInfo.h"
 #include "ARMBaseRegisterInfo.h"
 #include "ARMSubtarget.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/ScheduleDAG.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 using namespace llvm;
 
 static bool hasRAWHazard(MachineInstr *DefMI, MachineInstr *MI,
                          const TargetRegisterInfo &TRI) {
   // FIXME: Detect integer instructions properly.
   const MCInstrDesc &MCID = MI->getDesc();
   unsigned Domain = MCID.TSFlags & ARMII::DomainMask;
   if (MI->mayStore())
     return false;
   unsigned Opcode = MCID.getOpcode();
   if (Opcode == ARM::VMOVRS || Opcode == ARM::VMOVRRD)
     return false;
   if ((Domain & ARMII::DomainVFP) || (Domain & ARMII::DomainNEON))
     return MI->readsRegister(DefMI->getOperand(0).getReg(), &TRI);
   return false;
 }
 
+static bool IsAddress(const MachineOperand &MO) {
+  return MO.isGlobal() || MO.isSymbol() || MO.isCPI();
+}
+
+static bool IsAddressPair(const MachineOperand &MO0, const MachineOperand &MO1) {
+  if (MO0.getType() != MO1.getType())
+    return false;
+
+  switch (MO0.getType()) {
+  case MachineOperand::MO_ConstantPoolIndex:
+    if (MO0.getIndex() == MO1.getIndex())
+      break;
+    return false;
+  case MachineOperand::MO_GlobalAddress:
+    if (MO0.getGlobal() == MO1.getGlobal())
+      break;
+    return false;
+  case MachineOperand::MO_ExternalSymbol:
+    if (StringRef(MO0.getSymbolName()) == StringRef(MO1.getSymbolName()))
+      break;
+    return false;
+  default:
+    return false;
+  }
+  return MO0.getOffset() == MO1.getOffset();
+}
+
 ScheduleHazardRecognizer::HazardType
 ARMHazardRecognizer::getHazardType(SUnit *SU, int Stalls) {
   assert(Stalls == 0 && "ARM hazards don't support scoreboard lookahead");
 
   MachineInstr *MI = SU->getInstr();
 
+  const Triple TT(MI->getParent()->getParent()->getTarget().getTargetTriple());
+  if (TT.isOSWindows()) {
+    if (LastMI && LastMI->getOpcode() == ARM::t2MOVi16 &&
+        IsAddress(LastMI->getOperand(1))) {
+      // If there is a previous instruction of type mov.w of an address, there
+      // is a hazard for any instruction other than the mov.t that loads the
+      // high portion of the address as the Windows loader expects that the two
+      // instructions are contiguous.
+      assert(LastMI->getOperand(1).getTargetFlags() == ARMII::MO_LO16 &&
+             "expected low address load");
+      if (MI->getOpcode() == ARM::t2MOVTi16 &&
+          IsAddressPair(LastMI->getOperand(1), MI->getOperand(2)))
+        return NoHazard;
+      return Hazard;
+    }
+  }
+
   if (!MI->isDebugValue()) {
     // Look for special VMLA / VMLS hazards. A VMUL / VADD / VSUB following
     // a VMLA / VMLS will cause 4 cycle stall.
     const MCInstrDesc &MCID = MI->getDesc();
     if (LastMI && (MCID.TSFlags & ARMII::DomainMask) != ARMII::DomainGeneral) {
       MachineInstr *DefMI = LastMI;
       const MCInstrDesc &LastMCID = LastMI->getDesc();
       const TargetMachine &TM =

test/CodeGen/ARM/Windows/movw-movt-relocations.ll

+; RUN: llc -mtriple=thumbv7-windows -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-WINDOWS
+
+; RUN: llc -mtriple=thumbv7-eabi -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-EABI
+
+@i = common global i32 0, align 4
+@j = common global i32 0, align 4
+
+; Function Attrs: nounwind optsize readonly
+define i32 @relocation(i32 %j, i32 %k) {
+entry:
+  %0 = load i32* @i, align 4
+  %1 = load i32* @j, align 4
+  %add = add nsw i32 %1, %0
+  ret i32 %add
+}
+
+; CHECK-WINDOWS: movw r[[i:[0-4]]], :lower16:i
+; CHECK-WINDOWS-NEXT: movt r[[i]], :upper16:i
+; CHECK-WINDOWS: movw r[[j:[0-4]]], :lower16:j
+; CHECK-WINDOWS-NEXT: movt r[[j]], :upper16:j
+
+; CHECK-EABI: movw r[[i:[0-4]]], :lower16:i
+; CHECK-EABI: movw r[[j:[0-4]]], :lower16:j
+; CHECK-EABI-NEXT: movt r[[i]], :upper16:i
+; CHECK-EABI-NEXT: movt r[[j]], :upper16:j
+