This is an archive of the discontinued LLVM Phabricator instance.

[AIX][CodeGen] Storage Locations for Constant Pointers
ClosedPublic

Authored by qiongsiwu1 on Feb 16 2023, 7:25 AM.

Details

Summary

This patch adds an llc option -mroptr to specify storage locations for constant pointers on AIX.

When the -mroptr option is specified, constant pointers, virtual function tables, and virtual type tables are placed in read-only storage. Otherwise, by default, pointers, virtual function tables, and virtual type tables are placed are placed in read/write storage.

https://reviews.llvm.org/D144190 enables the -mroptr option for clang.

Diff Detail

Event Timeline

qiongsiwu1 created this revision.Feb 16 2023, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 7:25 AM
qiongsiwu1 requested review of this revision.Feb 16 2023, 7:25 AM

Clang format update.

myhsu added a subscriber: myhsu.Feb 16 2023, 12:48 PM

just wondering if there is any reason you chose not to implement this as a subtarget feature?

qiongsiwu1 added a comment.EditedFeb 17 2023, 6:01 AM

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

myhsu added a comment.Feb 20 2023, 9:43 AM

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

I think -mpcrel / pcrelative-memops in PPC is a good example. First, you can find pcrelative-memops's subtarget feature definition in llvm/lib/Target/PowerPC/PPC.td, which can be accessed by PPCSubtargetInfo::HasPCRelativeMemops. Then, in Clang, you can find mpcrel 's definition in clang/include/clang/Driver/Options.td. Such flag is put under the m_ppc_Features_Group so it will eventually be marshaled and piped into PPCTargetInfo::setFeatureEnabled (and some other PPCTargetInfo methods) in which you can translate it into LLVM pcrelative-memops target feature.

qiongsiwu1 added a comment.EditedFeb 22 2023, 8:11 AM

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

I think -mpcrel / pcrelative-memops in PPC is a good example. First, you can find pcrelative-memops's subtarget feature definition in llvm/lib/Target/PowerPC/PPC.td, which can be accessed by PPCSubtargetInfo::HasPCRelativeMemops. Then, in Clang, you can find mpcrel 's definition in clang/include/clang/Driver/Options.td. Such flag is put under the m_ppc_Features_Group so it will eventually be marshaled and piped into PPCTargetInfo::setFeatureEnabled (and some other PPCTargetInfo methods) in which you can translate it into LLVM pcrelative-memops target feature.

Ah I see! Thanks so much for the suggestion and clarification!

I took a closer look and discussed with other IBM colleagues and we think that we would like to keep the current implementation. The key reason is that this is not an PPC subtarget feature, rather a codegen feature for AIX/XCOFF. It has more to do with object file formats, linkers, and loaders. We are not offering this specific option on Linux.

Implementation-wise, as far as I can see, it is not easy to route the subtarget information to TargetLoweringObjectFileXCOFF, where we actually implement the logic for this option. A similar existing option is TargetOptions::IgnoreXCOFFVisibility.

That said, I may still be missing other aspects of the subtarget suggestion that could overcome the points above. Please let me know and I will look into them! Thanks again!

Gentle ping for review. Thanks!

stephenpeckham accepted this revision.Feb 23 2023, 1:13 PM

I don't see any issues with this code.

This revision is now accepted and ready to land.Feb 23 2023, 1:13 PM

Revise llc error checking. Adding a check for AIX.

myhsu accepted this revision.Feb 24 2023, 9:11 AM
llvm/docs/ReleaseNotes.rst
125–130

The requirement for use with data-sections is a design intent and not merely an implementation artifact. This may be a reasonable place to elaborate on that. Should also add additional comments in the code.

Updating the release note and adding comments on the data-sections check.

qiongsiwu1 marked an inline comment as done.Feb 28 2023, 10:19 AM
llvm/tools/llc/llc.cpp
502

Other calls to reportError do not appear to use periods at the end of the message.

506–509

Suggestion: Adjusted/expanded code comment

511

Same comment as above regarding ending full-stop.

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll
6

Please add --symbol-description (and observe that the storage mapping class is displayed as well).

23

Would OBJ-DAG allow the test to be written with the check lines in the order of the IR definitions?

30–32

Updating with the --symbol-description output would really help here.

39–42

dd is not the interesting at the IR level.
Also, all of the C++ implementation artifacts below are unchecked anyway.

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll
2

Same comments as for the 32-bit case.

qiongsiwu1 updated this revision to Diff 501870.Mar 2 2023, 7:49 AM

Thanks for the comments @hubert.reinterpretcast !! The patch is updated.

qiongsiwu1 marked 5 inline comments as done.Mar 2 2023, 7:53 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll
23

I suggest replacing all of the address and symbol table index fields with regular expressions.

29–30

The TOC entries are not interesting for our purposes here.

38

I think the rest of the file (including the definition of dd) can be removed.

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll
2

Same new comments as for the 32-bit case.

Ping for review. Thanks!

qiongsiwu1 updated this revision to Diff 502658.Mar 6 2023, 8:06 AM

Updating test cases to address review comments. A test for int *const x = (int *)0x1; is also added.

qiongsiwu1 marked 7 inline comments as done.Mar 6 2023, 8:07 AM

I think the implementation is fine, but just want to bring something else up for consideration wrt. to this option. As far as I can tell, nothing about this option is communicated in the IR. So if I split up IR generation (with clang -emit-llvm) and code generation (with opt/llc), the pointers will be placed into read-only sections only if -mroptr is specified to llc. While this isn't a common way users use the compiler, it may not be a concern. However, what are the implications of an LTO build. If we specify -mroptr on the compile steps but not on the link step, we won't get the behaviour. Presumably this is acceptable (i.e. the link will probably also fail if we forget to specify -flto on the link step) but it might be useful to document that somewhere.

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll
32

This patch is not expected to change the behaviour for x in any way. Should the x case really be included? If it is, should the test for it not be for it being RO even with -mno-roptr?

qiongsiwu1 added inline comments.Mar 8 2023, 6:17 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll
32

Thanks for the comment! This test was added because I thought we may expect the default behaviour of such pointers (that they are going into the RO section) to stay. If there is a change in such default behaviour, this test case can catch it. Maybe there are other tests covering this exact case but I am not sure about that.

It sounds like that we do not need to have a test for this behaviour since it is not changed by this patch. I am removing this test.

Address review comments - removing unnecessary tests.

This LGTM; thanks!

llvm/include/llvm/Target/TargetOptions.h
364

Can we rename this to XCOFFReadOnlyPointers?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2436

The option seems out of the control of the back-end to me (it is "user input"). I believe report_fatal_error may be more appropriate.

llvm/tools/llc/llc.cpp
513

See https://reviews.llvm.org/D144190#inline-1406700 for why having this diagnostic in llc may not be the best.

qiongsiwu1 updated this revision to Diff 503795.Mar 9 2023, 8:42 AM

Rename the ReadOnlyPointers option in the source to XCOFFReadOnlyPointers.

qiongsiwu1 marked an inline comment as done.Mar 9 2023, 8:49 AM
qiongsiwu1 marked an inline comment as done.
qiongsiwu1 marked an inline comment as done.Mar 9 2023, 3:18 PM
llvm/tools/llc/llc.cpp
513

Since the back end now errors out reliably (report_fatal_error), the comment linked above for the other patch implies that we can remove this llc diagnostic in this patch. Then we can land this patch first.

qiongsiwu1 added inline comments.Mar 15 2023, 6:42 AM
llvm/tools/llc/llc.cpp
513

Since the back end now errors out reliably (report_fatal_error), the comment linked above for the other patch implies that we can remove this llc diagnostic in this patch. Then we can land this patch first.

It seems that the report_fatal_error mechanism works more like an assert. llc crashes when encountering a report_fatal_error function. It is probably not ideal that we crash when -data-sections=false is passed through the command line. Should we keep this llc check? I am inclined to do so because I don't think we should allow llc to crash. That said, if the crashing behaviour is acceptable, I will remove the -data-sections=false test case, and remove this check. Alternatively, there may be other error reporting mechanisms in the backend that I am not aware of that can generate a valid error message without crashing. If there are, I will adopt them so we can report the error in the backend, and not crash.

Patch LGTM. Thanks for looking into the concerns.

llvm/tools/llc/llc.cpp
513

Since we have the diagnostic in llc (and for clang -cc1 in the other patch) already, and it is the most user-friendly behaviour, I think we are good to close on this.

Just noting that the report_fatal_error behaviour may be acceptable (in case we end up working on something similar in the future).

Update comments and release notes.

qiongsiwu1 marked 3 inline comments as done.Mar 15 2023, 10:37 AM

Fix clang-format.

@arsenm @MaskRay and @serge-sans-paille, I would like to draw your attention to this patch. We would like to get your advice on the following issues:

  1. We would like to do some error check in llc to guard against invalid option combinations. Doing the checks in llc's main feels out of place. Doing it in InitTargetOptionsFromCodeGenFlags() https://github.com/llvm/llvm-project/blob/4c82050c56926d840e4ccf253ad10e6ae3ee6cc7/llvm/lib/CodeGen/CommandFlags.cpp#L517 probably makes more sense, but we do not perform any error checks there at the moment. What is a good place to perform such error checks? This is related to https://reviews.llvm.org/D144190#4180329, where we are looking for advice on how to check codegen options when doing a pure linking job. What is the expected behaviour if an option that is not technically generic (such as this -mroptr option that only makes sense for XCOFF on AIX), but not architecture specific (in theory we may have XCOFF running on other platforms) either?
  2. We are touching some target-independent files to implement this option. Earlier @myhsu pointed out that this might be a subtarget feature https://reviews.llvm.org/D144189#4132940 and we argued that it is not https://reviews.llvm.org/D144189#4144677 because this is an AIX/XCOFF issue. We would like to notify you all and make sure you are ok with this as well.

Thanks so much!!

FYI @nemanjai @hubert.reinterpretcast

Ping for review/comment! Thanks so much!

Ping for review/comment! Thanks so much!

@arsenm @MaskRay and @serge-sans-paille, if there are no further feedback/comments, I will land this patch tomorrow (March 23, 2023). Thanks so much for your help!

MaskRay accepted this revision.Mar 22 2023, 11:19 AM
MaskRay added inline comments.
llvm/docs/ReleaseNotes.rst
129

s/available/only supported/

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll
18

;CHECK => ; CHECK

22

Replace {{[0-9]+}} with [[#]]

Address code review. Thanks @MaskRay for the comments and approval!

qiongsiwu1 marked 3 inline comments as done.Mar 22 2023, 11:53 AM

No comment, thanks for the patch!

This revision was automatically updated to reflect the committed changes.