This is an archive of the discontinued LLVM Phabricator instance.

[LTO][AIX] Invoking AIX System Assembler in LTO CodeGen
ClosedPublic

Authored by qiongsiwu1 on Sep 21 2022, 10:23 AM.

Details

Summary

This patch teaches LTOCodeGenerator to call into the AIX system assembler to generate object files. This is in contrast to the approach taken on other platforms, where the LTOCodeGenerate calls the integrated assembler to generate object files. We need to rely on the system assembler because the integrated assembler is incomplete at the moment.

Diff Detail

Event Timeline

qiongsiwu1 created this revision.Sep 21 2022, 10:23 AM
qiongsiwu1 requested review of this revision.Sep 21 2022, 10:23 AM
qiongsiwu1 edited the summary of this revision. (Show Details)Sep 21 2022, 10:26 AM

Update the diff to include the context.

w2yehia added inline comments.Sep 21 2022, 12:58 PM
llvm/lib/LTO/LTOCodeGenerator.cpp
119

We should either introduce the savetemps option in a separate patch (my preference), or implement it fully by calling Config.addSaveTemps in the LTOCodeGenerator constructor.
Also, the option spelling should be -save-temps to match the existing option available in other tools.

MaskRay added inline comments.Sep 21 2022, 10:42 PM
llvm/lib/LTO/LTOCodeGenerator.cpp
120

Drop the default value cl::init(false)

246

The code is AIX specific. The function should be named so instead of pretending it to be generic and used by other targets.

255

The code is AIX specific. The function should be named so instead of pretending it to be generic and used by other targets.

260

Avoid getpid. See createTemporaryFile in Support/FileSystem.h

Addressing code review:

  1. The lto-save-temp option is removed. We will add that later with a different patch.
  2. The newly added AIX specific functions are explicitly named.
  3. Minor changes to variable names (runSystemAssembler(SmallString<128> &Name) -> runSystemAssembler(SmallString<128> &AssemblyFile)) and removing unnecessary comments.

Thanks for the feedback @w2yehia and @MaskRay !

Minor formatting update.

qiongsiwu1 marked 5 inline comments as done.Sep 22 2022, 5:49 AM

Updating function names so they are more explicit.

Gentle ping for review. Thanks!

w2yehia accepted this revision.Sep 26 2022, 7:58 PM

LGTM

llvm/lib/LTO/LTOCodeGenerator.cpp
298

braces can be removed

This revision is now accepted and ready to land.Sep 26 2022, 7:58 PM
MaskRay added inline comments.Sep 26 2022, 8:16 PM
llvm/lib/LTO/LTOCodeGenerator.cpp
120

This option needs an AIX specific name.

251

Remove this function. Inline its use.

Addressing code review comments.

qiongsiwu1 marked 3 inline comments as done.Sep 27 2022, 6:54 AM
MaskRay accepted this revision.Sep 27 2022, 3:23 PM
MaskRay added inline comments.
llvm/lib/LTO/LTOCodeGenerator.cpp
279

Too many blank lines which don't improve readability. The function has some small number of logically inherent units and within a unit a blank line may be unneeded.

Addressing code review comments and fixing a test failure on Linux. I will land patch once the pre-commit CI finishes.

Thanks for the review!

qiongsiwu1 marked an inline comment as done.Sep 28 2022, 7:35 AM