ping.

In D103696#2816619, @EGuesnet wrote:

Hello,
For you information, we have had trouble with arcanist configuration, and my account is used to push to review. Patch has been written by @MaryamBen, as confirmed by commit author.
Comments in push message are also made @MaryamBen.
Regards.

Thanks for the updates Xing, LGTM.

One comment but otherwise LGTM.

Thanks for working on this. I'm excited to finally replace all the precompiled objects we used for testing the XCOFF tools implementation. I've only got one initial comment since I'm not real familiar with this part fo the code yet, but will spend some more time going over this tomorrow.

In D103696#2803289, @jsji wrote:

I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

https://reviews.llvm.org/D100375 is ready for review, so I would suggest we use yam2obj base on that .
It would be great if we can help review and test D100375.

Thanks for working on this. My first suggestion is we need to break up this patch to make it smaller and easier to review. The natural place to partition it is the llvm-readobj functionality. I don't think the YAML tools support generating 64-bit XCOFF objects yet, but we can add a precompiled object file as test input.

Ping.

LGTM.

Thanks Xing, LGTM.

Thanks for the updates Victor, LGTM.

In D101298#2723019, @xingxue wrote:

This version is clean of format changes made by clang-format.

Great work Sidharth, a couple minor nits but otherwise the patch LGTM.

In D101545#2726036, @MaskRay wrote:

I keep feeling that the AIX implementation probably needs its own file. Making Itanium/ARM/SEH share some functions actually make generic code improvement harder.

Awesome work Sidarth, patch looks really good. I've got one initial comment.

One minor nit but otherwise LGTM.

Thanks for the quick updates. Patch is getting really close I think.

Thanks for splitting this out Xing. I suggest showing the syntax differences between gas and AIX assembly we intend to work around with these macros in the commit message. LGTM.

Patch looks pretty good. IIUC we were not printing the '@gd' specifier before because it is assumed by default by the system assembler. Now that we are adding object file support we can no longer rely of the system assembler to assume the specifier based on the storage mapping class ,and need to add the new variant kinds so we create the correct relocations, which has the side effect of also changing the asm we emit. I know I am being pedantic, but I think we should land that part of the change and the update to the asm test in an initial patch even though the object writing is the motivation for adding it.

In D91426#2569605, @stefanp wrote:

In D91426#2568567, @sfertile wrote:

Could there be a simpler approach where we always emit a .got section on PPC64: In addReservedSymbols if we fail to resolve .TOC. then we create it?

We could do that and it would certainly solve the problem. The issue I have with that is that we will now have a .got for everything we link including things that we used to link in the past that did not require it. It seems odd to say that since the design of the linker won't allow us to do this easily we will generate this section whether we need it or not.

LGTM.

discussed with sean offline, the we do not call the emitGlobalDtorWithCXAAtExit() in AIX. so we do not have the problem for the "__dso_handle"

I initially had concerns with the places clang introduces non-default visibility in codegen. The few cases I was worried we would hit were related to static init , but that was before @Xiangling_L static init related work. After those changes I have no other concerns. I would wait a day or two to ensure that there are no more comments related to the round-trip-args behaviour but otherwise LGTM.

Thanks for the updates Lei. LGTM.

One minor comment but otherwise LGTM.

• clang-formated lambda
• changed check-prefixes to check-prefix
• added whitespace between 32-bit and 64-bit check lines
• Changed check for further custom RegLcos to include that the ValNo of the next RegLoc is the same as the original ValNo.

Patch looks good Lei, the one thing we are missing though is a lit test that checks for the expected toc entries.

LGTM.

Fix formating on assert.

• Add asserts that the ValNo of custom RegLocs match the ValNo of the custom MemLoc.
• Assert that we are targeting 32-bit codegen when we have 4 custom RegLocs.
• Add comments to the tests to make it easier to determine what is being tested.

• Converted loop to explicit handling of custom RegLocs
• Added const on member function.

Fixed a number of formatting and spelling mistakes.

Could there be a simpler approach where we always emit a .got section on PPC64: In addReservedSymbols if we fail to resolve .TOC. then we create it?

LGTM.

LGTM. Please wait until David has given it the OK too though.

Thanks for the update. LGTM.

I'll need a bit more time to finish going through ppc-shrink-wrapping.ll but the patch is looking really good. Thanks for the updates.

Addressed review comments.

ping.