Page MenuHomePhabricator

Boilerplate for producing XCOFF object files from the PowerPC backend.
Needs ReviewPublic

Authored by sfertile on May 8 2019, 1:01 PM.

Details

Summary

Stubs out a number of the classes needed to produce a new object file format (XCOFF) for the powerpc-aix target. For testing input is an empty module which produces an object file with just a file header.

Diff Detail

Event Timeline

sfertile created this revision.May 8 2019, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 1:01 PM
sfertile marked an inline comment as done.May 17 2019, 9:53 AM
sfertile added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1667

Just realized that this will change the AsmPrinter that will get created for triples like -mtriple=ppc32-- where no OS is specified. I'll fix this so Linux is still the default AsmPrinter.

I'm not super familiar with the MC layer but overall this looks good to me.
I have a couple of minor comments.

Also, it seems that there is a part of that patch that is missing. The test added: aix-xcoff-basic.ll causes an assert with top of trunk.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.h
41

Why don't we pass in the is64Bit flag and the Triple into the function?
I know that we don't intend to support 64 bit from the start but I assume we will want to support it at one point right? We can check the flag at the start and if it is 64 bit we can emit a fatal_error or unreachable as you have done in other places.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
87

So, previously the default was ELF now it is XCOFF.
This probably won't make much of a difference but ideally we should be spelling out what we support.

if (TheTriple.isOSDarwin())
  MAI = new PPCMCAsmInfoDarwin(isPPC64, TheTriple);
else if (TheTriple.isOSBinFormatELF())
   MAI = new PPCELFMCAsmInfo(isPPC64, TheTriple);
else if (TheTriple.isOSBinFormatXCOFF())
   MAI = new PPCXCOFFMCAsmInfo();
else
   llvm_unreachable("Unknown binary format.");

However, I don't know if the above is safe. Some code may rely on having ELF as default. Either way, I don't think we can leave it as XCOFF default.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h
59

Here too maybe we want to keep the 64 bit flag?

hfinkel added inline comments.Tue, May 28, 8:54 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
68

Will the tools on AIX complain if this is 0? Our general policy is that the compiler's output should be deterministic/reproducible and this breaks that. Looks like D23934 was never finished, but this value should be tied to that (once it's finished).

sfertile marked 2 inline comments as done.Wed, May 29, 5:50 AM
sfertile added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
68

No a zero value should be fine. Is the goal the compiler always produces reproducible output by default, or just that we can enable reproducible output with an option?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h
59

Yeah, good idea. The first few patches for the Objectwriter are 32-bit only but we can simply report an error on 64-bit output for now.

hfinkel added inline comments.Wed, May 29, 7:55 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
68

Interesting question. I'd say by default, but I don't know that we've had a situation before where the platform default is to produce non-reproducible outputs at a low level (e.g., in the object-file format).

I recommend that we just set this to zero for now, and then have a separate discussion on how to change this later as necessary and/or desired.

thakis added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
68

If 0 works fine here, then go with that. But there's precedent for writing the current time into the output by default: clang-cl does that, since link.exe /incremental silently mislinks if that field is 0. So we do the safe thing by default and only write a 0 if the user explicitly passes /Brepro to clang-cl (which finally makes Asm.isIncrementalLinkerCompatible() return false here: http://llvm-cs.pcc.me.uk/lib/MC/WinCOFFObjectWriter.cpp#1057 )

llvm/include/llvm/MC/MCAsmInfoXCOFF.h
1

This is missing the - between the filename and the description.

llvm/include/llvm/MC/MCXCOFFStreamer.h
30

I don't think overriding a function just to call the direct base version of it is necessary.

llvm/include/llvm/Support/TargetRegistry.h
521

The use of the Register*Streamer functions is associated with registering the appropriate streamer factory function for the object file format given a specific architecture in lib/Target/*/MCTargetDesc. If there are no classes derived from MCXCOFFStreamer, then I don't think we need this.

llvm/lib/MC/MCAsmInfoXCOFF.cpp
1

Refer to the previous comment regarding a missing -.

llvm/lib/MC/MCXCOFFStreamer.cpp
30

If the intent is to fail with assertions but silently continue in release, then this should be
assert(false && "Not implemented yet");

Please review other uses of llvm_unreachable in this patch.

llvm/lib/MC/XCOFFObjectWriter.cpp
19

As a class defined in a .cpp file, this should probably go in an unnamed namespace.

23

The llvm:: here seems noisy and should be unnecessary here (and in a good number of cases in this file).

43

Does this need a TODO?

72

int32_t

74

uint16_t

76

uint16_t

llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
20

Use // end anonymous namespace.

22

Use = default.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1652

Granted, this does not override a function just to clearly invoke the implementation in the direct base class, but explicitly skipping to an indirect base class with no comment is not better. Also, the direct base class does not override AsmPrinter::doFinalization anyway.