Extended the patch so that lazy object files are handled the same as archives, and bitcode files are handled same as native objects, both in archives and as lazy object files.

One minor nit, but otherwise LGTM.

Previous patch was not really NFC, since we were inserting VRSAVE pseudos on AIX, then emiting a fatal error in frame lowering. This was because we missed changing the condition gating the vrsave insertion to include the AIX abi.

One minor nit, but otherwise LGTM. If Nemanja would still like the comment he suggested make sure to address that before committing.

I've spent some time reading through the linked thread. Thanks. There is a lot to reply to so I hope I don't miss anything.

• Removed now unused local variable.
• Removed -framepointer=all option in lit test.
• Fixed formatting of runsteps in new lit test.

In D85994#2223415, @stefanp wrote:

I am unable to use the assembly files. The problem is that __tls_get_addr is a special name for the PPC backend and we cannot generate an object file from incorrect assembly. For example:

$ cat GeneralDynamic.s
	.text
GeneralDynamic:                         # @GeneralDynamic
	addis 3, 2, x@got@tlsgd@ha
	addi 3, 3, x@got@tlsgd@l
	bl __tls_get_addr()
	blr

Produces:

$ llvm-mc GeneralDynamic.s --filetype=obj -o /dev/null
GeneralDynamic.s:5:20: error: unknown token in expression
        bl __tls_get_addr()
                          ^
GeneralDynamic.s:5:20: error: invalid TLS call expression
        bl __tls_get_addr()
                          ^

LGTM.

In D84360#2215944, @nemanjai wrote:

But that's what check_LD_STD_W_PADDI does. Do you mean that I should add checks for the actual values?

I believe we can have the optimization relocation on a code sequence like:

paddi ra, sym@pcrel
load/store rt, offset(ra)

If so we should have a test showing we relocate it properly even if we don't optimize it yet.

LGTM.

LGTM.

LGTM.

Hey Digger, sorry for letting this sit for so long. Feel free to ping reviews periodically to get peoples attention back on them.

LGTM.

One comment but otherwise LGTM.

I think we are really close now. I would suggest having a second lit test, which we link into either a static exec or pie exec which has calls using the R_PPC64_REL24_NOTOC relocation to symbols with global linkage and default visibility. I think it can be done with a single file (ie define all the callers and callees in the same file, no need for a second input file).

LGTM.

Patch looks very good Victor. I've left a couple minor comments in the implementation. I have a few suggestions in relation to the testing.

LGTM. Thank Stefan.

LGTM.

Looks good Zarko. I think we should either flip this over to show the stack growing down, or keep it as is but add High Memory at the bottom and Low memory at the top to show the stack grows downwards.

Abandoning based on the discussion in D81457.

LGTM.

Do you want to add a test for overflow of a 34-bit value?

LGTM.

In D81106#2090619, @MaskRay wrote:

Shall we mask out the bits (keep existing (read32(loc) & ~mask))? If other relocations don't behave this way, let's fix it. Writing zeros to the bits is a courtesy of the assembly but should we make use of the assumption? PPC64 is a RELA target and for an RELA target the implicit addend should be relied upon. AArch64.cpp masks out the bits as well.

The CHECK-O section prints out the relocation and then it has symbol+<addend>. Is that what you are looking for?

Yeah, not sure how I missed that the first time. Sorry.

Minor nit not related to this patch is that the test is really noisy. I realize this is the assembly clang emits, but a lit test can be cleaned up to just the needed constructs. Removing the comments, zero padding after the functions, local aliases (.Lexternal$local) and finally the size directives and the labels that are only needed for those directives goes along way in creating a much simpler test that helps to highlight exactly what functionality is being exercised at a glance. After landing this patch can you do an NFC cleanup of the test?

LGTM.

Do you already have a test which dumps the relocations and verifies the addend on the relocations? (Do the llvm tools support dumping these new relocations?)

One comment but otherwise LGTM.

Thanks for implementing this MaskRay. I've left a couple comments, but still need to go through the testing.

rebased to pick up NFC commit.

• Removed extra whitespace.
• Removed redundant testing (of fprs saves)

ping.

ping.

👍 Thanks for trying this out Digger, I think this approach is the right one to take. I've had an initial look and left a few comments inline. I'll need to spend some time understanding ExternalSymbolSDNodes a bit better before being able to review the patch thoroughly enough to approve.

Updated aix-calleesaved.ll test.

LGTM.

LGTM.

One minor comment but otherwise LGTM. Thanks for fixing this.

In D78431#2010446, @MaskRay wrote:

@sfertile Your revert does not include Differential Revision: . I guess if it included, the revision would be automatically re-opened, and Commits: would list the associated revert commits.

Have you considered not transforming ExternalSymbolSDNodes, but instead supporting them for AIX and then creating the symbol and emitting the linkage for the ExternalSymbol machine operand in the ASMPrinter? The approach this patch takes adds complexity to a target independent part of the MC infrastructure. The amount of complexity it adds is not unreasonable, but I think if we can contain the complexity to the PowerPC backend its worth considering strongly even if the implementation there is more difficult.

I had to pull this as it breaks the PPC mutlistage LLD bot. Consider the following input: