Page MenuHomePhabricator

[SystemZ][z/OS] Add GOFF Support to the DataLayout
ClosedPublic

Authored by anirudhp on Sep 7 2021, 6:55 AM.

Details

Summary
  • This patch adds in the GOFF mangling support to the LLVM data layout string. A corresponding additional line has been added into the data layout section in the language reference documentation.
  • Furthermore, this patch also sets the right data layout string for the z/OS target in the SystemZ backend.

Diff Detail

Event Timeline

anirudhp created this revision.Sep 7 2021, 6:55 AM
anirudhp requested review of this revision.Sep 7 2021, 6:55 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 7 2021, 6:55 AM
anirudhp edited reviewers, added: Everybody0523; removed: neumannh.Sep 7 2021, 7:15 AM

SystemZ specific parts LGTM, but it would be good to have someone else review the common code / IR changes as well.

anirudhp updated this revision to Diff 371106.Sep 7 2021, 9:46 AM
  • Condensed the lit test a bit.
Kai accepted this revision.Sep 7 2021, 9:47 AM

LGTM. Thanks for the documentation update.

This revision is now accepted and ready to land.Sep 7 2021, 9:47 AM

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

At a future point, we will be changing the local and global prefixes. At that point we would still need a separate MM_GOFF field. I feel it would be a bit better to enforce it from now itself.

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

At a future point, we will be changing the local and global prefixes. At that point we would still need a separate MM_GOFF field. I feel it would be a bit better to enforce it from now itself.

I see. Is there a reason why we cannot use the "correct" prefixes to begin with?

pmatos resigned from this revision.Sep 15 2021, 4:19 AM
anirudhp updated this revision to Diff 372733.Sep 15 2021, 9:40 AM
  • Introduce separate label prefix for MM_GOFF

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

At a future point, we will be changing the local and global prefixes. At that point we would still need a separate MM_GOFF field. I feel it would be a bit better to enforce it from now itself.

I see. Is there a reason why we cannot use the "correct" prefixes to begin with?

Nope. I'll update the patch.

Looking at the common code parts, it seems the behavior of MM_GOFF is actually identical to MM_ELF. Is this correct? If so, do we really need a different format type here?

At a future point, we will be changing the local and global prefixes. At that point we would still need a separate MM_GOFF field. I feel it would be a bit better to enforce it from now itself.

I see. Is there a reason why we cannot use the "correct" prefixes to begin with?

Nope. I've added it. There should also be a change to the MCAsmInfo fields (PrivateGlobalPrefix and PrivateLabelPrefix). These will be added in when the MCAsmInfo interfaces for z/OS are put up.

uweigand added inline comments.Sep 15 2021, 9:52 AM
llvm/docs/LangRef.rst
2608

Comment needs to be update now.

anirudhp updated this revision to Diff 372737.Sep 15 2021, 9:59 AM
  • Update to the documentation as well in account for the update to the global prefix.
anirudhp marked an inline comment as done.Sep 15 2021, 9:59 AM
uweigand accepted this revision.Sep 15 2021, 10:17 AM

This LGTM now.

MaskRay added inline comments.Sep 16 2021, 9:45 AM
clang/test/CodeGen/target-data.c
232

If you add so many RUN lines at once, please use unittests instead. This would cost some test execution time

anirudhp added inline comments.Sep 20 2021, 7:54 AM
clang/test/CodeGen/target-data.c
232

We're essentially matching what was available for the systemz elf target (above lines) for the z/OS target. Is there a real concern for the execution time here for moving it to a separate unit test. If we did, it would seem to "stand out" for just the z/OS target.

MaskRay added inline comments.Sep 20 2021, 1:17 PM
clang/test/CodeGen/target-data.c
232

This is a real concern. Perhaps you can also move the SYstemZ ELF tests to a unit test.

MaskRay added inline comments.Sep 20 2021, 1:19 PM
clang/test/CodeGen/target-data.c
232

It also doesn't appear useful to enumerate every combination when the patch doesn't touch these combination individually.

I.e. if you add z/OS condition to arch8 code, then having a test is fine. If you don't touch it, I don't think you should enumerate {elf,goff} * {z10,arch8,z196,arch9,...}

anirudhp updated this revision to Diff 373999.Sep 21 2021, 11:48 AM
  • Reduced the number of test lines in target-data.c, since we don't have to check for every combination of arch,cpu for the SystemZ target (for both elf and z/OS)
  • Reduced the number of test lines in target-data.c, since we don't have to check for every combination of arch,cpu for the SystemZ target (for both elf and z/OS)

These changes still LGTM from a SystemZ target perspective.

MaskRay accepted this revision.Sep 21 2021, 11:58 AM

Thanks for cleaning up RUN lines :)

anirudhp marked 2 inline comments as done.Sep 21 2021, 12:03 PM
anirudhp added inline comments.
clang/test/CodeGen/target-data.c
232

After a bit of discussions with @uweigand , we've reduced the number of run lines for the SystemZ target(s) (both elf and goff). They should be greatly reduced now.

This revision was landed with ongoing or failed builds.Sep 24 2021, 11:09 AM
This revision was automatically updated to reflect the committed changes.
anirudhp marked an inline comment as done.