Page MenuHomePhabricator

[SystemZ][z/OS] Initial code to generate assembly files on z/OS
ClosedPublic

Authored by anirudhp on Jul 20 2021, 10:08 AM.

Details

Summary
  • This patch consists of the bare basic code needed in order to generate some assembly for the z/OS target.
  • Only the .text and the .bss sections are added for now.
  • The relevant MCSectionGOFF/Symbol interfaces have been added. This enables us to print out the GOFF machine code sections.
  • This patch enables us to add simple lit tests wherever possible, and contribute to the testing coverage for the z/OS target
  • Further improvements and additions will be made in future patches.

Diff Detail

Event Timeline

anirudhp created this revision.Jul 20 2021, 10:08 AM
anirudhp requested review of this revision.Jul 20 2021, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2021, 10:08 AM

Looks good in general, just a few suggestions. The commit message/description should say specifically what functionality this adds.

llvm/include/llvm/MC/MCContext.h
281

I don't think this type is necessary, you could use StringRef directly in the map.

llvm/include/llvm/MC/MCSectionGOFF.h
33

Is this necessary?

38

It might be a good idea to generalise printName from MCSectionELF.cpp to handle outputting names in quotes with proper escaping.

llvm/lib/MC/MCContext.cpp
621–633

Something like this should do the same, assuming the key is changed to StringRef

llvm/test/CodeGen/SystemZ/zos-simple-test.ll
4

The test doesn't seem directly related to the GOFF section code added here. e.g. it doesn't check any section output, but does check an instruction not added in this patch.

anirudhp updated this revision to Diff 360496.Jul 21 2021, 9:22 AM
anirudhp marked 3 inline comments as done.
  • Addressing comments
llvm/include/llvm/MC/MCContext.h
281

Hmm you are correct, since there is only field in the key. But taking into account future maintainability, to me, this seems better to leave in as is.

llvm/include/llvm/MC/MCSectionGOFF.h
33

Nope. You're right. There doesn't seem to be a reason for having a virtual destructor here. The default destructor is enough.

38

Sure. Done.

llvm/test/CodeGen/SystemZ/zos-simple-test.ll
4

I have added a uninitialized global variable to grep for the .bss section and added a new check string(s) to check for both of them.

anirudhp edited the summary of this revision. (Show Details)Jul 21 2021, 9:27 AM
anirudhp marked 2 inline comments as done.Jul 21 2021, 9:29 AM
anirudhp added inline comments.
llvm/test/CodeGen/SystemZ/zos-simple-test.ll
4

In regards to the instruction check, the instruction already exists (we're using existing SystemZ instructions), if the check itself makes it confusing I can remove it.

anirudhp added inline comments.Jul 21 2021, 3:18 PM
llvm/include/llvm/MC/MCSectionGOFF.h
38

Sorry I just realized I didn't quite answer your question properly. Yes, I will port over the complete printName implementation from the ELF part over.

However, when looking at it, I realized there;s a bug with this patch. The GOFF sections are printed, but the ELF section are printed too, and this doesn't quite make sense. There shouldn't be mixing of sections. So I will try to resolve that first.

anirudhp updated this revision to Diff 360884.Jul 22 2021, 10:50 AM
  • Introduced GOFF support in the TargetLoweringObjectImpl phase, so that we correctly deduce the sections to print out, and not fall back on the default ELF sections.
  • This was what was happening previously, with a mix of both GOFF sections and ELF sections.
tmatheson added inline comments.Jul 23 2021, 4:21 AM
llvm/include/llvm/MC/MCSectionGOFF.h
38

The only change I can see from the first revision is that instead of OS << getName() it is now printing each character individually? This will be slower and more verbose than the original.

The printName function in MCSectionELF.cpp is iterating over individual characters in the name so that it can correctly quote them, e.g. if the name is xx"yy it will be printed as "xx\"yy". Since this is a common operation (it's already copy-pasted verbatim to MCSectionWasm.cpp) it seems sensible to move it somewhere commonly accessible and call it from all three MCSectionXXX.cpp.

If you think doing that is outside the scope of this work then please restore the original version.

llvm/lib/MC/MCContext.cpp
621–633

This comment still applies (even without getting rid of GOFFSectionKey)

anirudhp updated this revision to Diff 361721.Jul 26 2021, 10:46 AM
  • For the GOFFUniquingMap map, replace the key from a struct to a standard std::string as the Key. This should eliminate the need for the struct and condenses the getGOFFSection function a bit more
anirudhp marked an inline comment as done.Jul 26 2021, 10:51 AM
anirudhp added inline comments.
llvm/include/llvm/MC/MCSectionGOFF.h
38

Ahh I see what you mean. I think for now, I'd rather just keep it the way it was originally (i.e. not looping through it character by character). I would possibly keep the refactoring of the printName function so that it can be used by MCSection[ELF|GOFF|Wasm] as a separate patch if that's okay?

llvm/lib/MC/MCContext.cpp
621–633

Looking at it further. I followed your previous comment and removed the GOFFSectionKey struct and used as a standard std::string as the Key. We do have code that we plan to put up in the next series of patches, which would make a StringRef as the key not work, hence I opted for a standard std::string.

tmatheson accepted this revision.Jul 27 2021, 1:51 AM
This revision is now accepted and ready to land.Jul 27 2021, 1:51 AM
This revision was landed with ongoing or failed builds.Jul 27 2021, 8:29 AM
This revision was automatically updated to reflect the committed changes.
anirudhp marked an inline comment as done.