[PATCH] [AArch64] Handle weak references to variables in small model when using the static relocation model

[Renato Golin]

Hi Keith,

I'll be guessing a bit here, so take my reviews with a pinch of salt.

Also, if you could diff -U99 and post on Phab, that'd help the patch review.


On 29 August 2014 14:42, Keith Walker <kwalker at arm.com> wrote:
> This patch addresses the special case when loading the address of a variable
> that has a weak reference when in small model and we are using the static
> relocation model.   This is a corner case where the code might be executing
> too high in memory to be able to generate a zero value if we use the usual
> ADRP + ADDX sequence when the weak reference is not satisfied.

The rationale makes sense to me. Some comments inline...

+  if ((OpFlags & AArch64II::MO_CONSTPOOL) != 0) {
+    assert(getTargetMachine().getCodeModel() == CodeModel::Small &&
+           "use of MO_CONSTPOOL only supported on small model");
+    SDValue Hi = DAG.getTargetConstantPool(GV, PtrVT, 0, 0,
AArch64II::MO_PAGE);

this violates 80-col restriction.


+    unsigned char LoFlags = AArch64II::MO_PAGEOFF | AArch64II::MO_NC;
+    SDValue Lo = DAG.getTargetConstantPool(GV, PtrVT, 0, 0, LoFlags);
+
+    SDValue ADRP = DAG.getNode(AArch64ISD::ADRP, DL, PtrVT, Hi);
+    SDValue PoolAddr = DAG.getNode(AArch64ISD::ADDlow, DL, PtrVT, ADRP, Lo);

According to my ARM ARM, ADRP stores the address in a
high:low:Zero(12) if it's a page address. I'm guessing your case here
is not. The ARM ARM doesn't tell how to set the op via assembly
mnemonics, though.


+    SDValue GlobalAddr = DAG.getLoad(PtrVT, DL, DAG.getEntryNode(), PoolAddr,
+                                     MachinePointerInfo::getConstantPool(),
+                                     /*isVolatile=*/ false,
+                                     /*isNonTemporal=*/ true,
+                                     /*isInvariant=*/ true, 8);
+    if (GN->getOffset() != 0)
+      return DAG.getNode(ISD::ADD, DL, PtrVT, GlobalAddr,
+                         DAG.getConstant(GN->getOffset(), PtrVT));

Is this constant pool + offset guaranteed to be within acceptable
distances? Could you get into a situation that your fix would fall
into the same trouble, say if you had a large enough constant pool?

Thanks!
--renato