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

[Asiri Rathnayake]

Hi Renato, Tim,

I thought I'd chase this up as Keith is on holiday. Please see my comments
below:

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Renato Golin
Sent: 31 August 2014 00:35
To: Keith Walker

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

I've made Phab review here: http://reviews.llvm.org/D5285

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

Hmmm, I count exactly 80 columns here.

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

I think you are referring to the semantics of the assembly instruction (how
the CPU interprets the instruction), whereas the assembly mnemonics are
setup this way. I've checked with the old ARM64 backend (+ armcc) and this
is the correct way to setup the operation.

>> +    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?

My understanding of the AArch64 backend (and llvm for that matter) is little
limited, but this is how the old ARM64 backend used to load weak references
in the absence of a GOT. This is a regression from the backend merge.

The small model allows a +/- 4GB address space, which would require a very
large code segment or an equally large constant pool in order for the symbol
to be out of reach, which I think is very unlikely. I cannot think of a
simple test case that would force that kind of a scenario, but my guess is
that the linker will error when it detects such an out of range reference.
Also, the user would be partially responsible for using the small model when
the program is that large.

Does this make sense? Ok to commit?

Thanks.

- Asiri