[PATCH] PowerPC support for the ELFv2 ABI (powerpc64le-linux)

[Ulrich Weigand]

Hal Finkel <hfinkel at anl.gov> wrote on 19.07.2014 00:02:08:

> Hi Uli,
>
> I've commented on each of the patches below...

Thanks again for the review!


> > (See attached file: diff-llvm-elfv2-abiversion)
>
> LGTM.

Now checked in as revision 213484.


> > This can be improved upon by modifying
> > common code to pass the target symbol when calling
> > needsRelocateWithSymbol
> > (probably best done as a follow-on patch).
>
> Yes, please provide such a patch.

Checked in as revision 213487.

> > (See attached file: diff-llvm-elfv2-localentry)
>
> +static inline int64_t
> +PPC64_LOCAL_ENTRY_OFFSET(unsigned Other) {
> +  unsigned Val = (Other & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT;
> +  return ((1 << Val) >> 2) << 2;
> +}
> +static inline unsigned
> +PPC64_SET_LOCAL_ENTRY_OFFSET(int64_t Offset) {
> +  unsigned Val = (Offset >= 4 * 4
> +                  ? (Offset >= 8 * 4
> +                     ? (Offset >= 16 * 4 ? 6 : 5)
> +                     : 4)
> +                  : (Offset >= 2 * 4
> +                     ? 3
> +                     : (Offset >= 1 * 4 ? 2 : 0)));
> +  return Val << STO_PPC64_LOCAL_BIT;
> +}
> +
>
> inline functions are fine, but please follow the LLVM coding
> convention for naming them (and specifically, they should not look
> like macros).

OK, I've now renamed those to decodeLocalEntryOffset and
encodeLocalEntryOffset.

> Otherwise, LGTM.

Checked in as revision 213485.


> > (See attached file: diff-llvm-elfv2-funcdesc)
>
> +/// EmitFunctionBodyStart - Emit a global entry point prefix for ELFv2.
> +void PPCLinuxAsmPrinter::EmitFunctionBodyStart() {
> +  if (Subtarget.isELFv2ABI()
> +      && !MF->getRegInfo().use_empty(PPC::X2)) {
>
> Please add a comment here explaining this check -- some of the text
> above would be good ;)

Done.

> Otherwise, LGTM.

Checked in as revision 213489.


> > (See attached file: diff-llvm-elfv2-stack)
>
> This last part makes me a bit uncomfortable. Clang is not the only
> frontend, and placing a restriction on byval like that, which is a
> fairly generic parameter attribute seems unnecessary. Moreover, the
> Clang change and whether we support byval seems orthogonal. Can't we
> keep the Clang change (which is an optimization), and also have a
> slow-path byval implementation that uses a local stack slot when
> necessary? I suspect the answer is yes, and if I'm right, please
> implement that.

OK, I've reimplemented that bit as suggested.  This was a bit more
involved than I thought; I first had to refactor the existing handling
of the various stack objects to allow for using a local stack slot
instead, and that refactoring uncovered a pre-existing missed-
optimization bug in SelectAddressRegImm ...

I've checked in a fix for that bug as revision 213482 and the
refactoring as revision 213483.

> +  // area, and parameter passing area.  We start with at least 48/32
bytes,
>
> Please write 48(ELFv1)/32(ELFv2) bytes.

Sure.

> Otherwise, LGTM.

Checked in as revision 213490.


> > (See attached file: diff-llvm-elfv2-crsave)
>
> Please add FIXME near the current code-generation log.ic (in
> PPCFrameLowering::emitPrologue?).

Done.

> Otherwise, LGTM.

Checked in as revision 213492.


> > (See attached file: diff-llvm-elfv2-dyld)
>
> +          Value.Addend += ELF::PPC64_LOCAL_ENTRY_OFFSET (SymOther);
>
> Remove the space after OFFSET (and fix the function name as noted
earlier).

Fixed.

> Otherwise, LGTM.

Checked in as revision 213491.


Thanks,
Ulrich