[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. https://reviews.llvm.org/D150586 has landed and the problematic test case is removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Patch to remove the problematic test case. https://reviews.llvm.org/D150586 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. This patch is causing some buildbot failures. Specifically https://lab.llvm.org/buildbot/#/builders/139/builds/40652 https://lab.llvm.org/buildbot/#/builders/258/builds/810 I am looking into them. Please revert if this cannot wait. Thanks! Repository: rG LLVM

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9715af434579: [AIX][clang] Storage Locations for Constant Pointers (authored by qiongsiwu1). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Pre-commit CI x64 Debian is failing https://buildkite.com/llvm-project/premerge-checks/builds/152189 because the build machine's cmake version is too old. I am landing this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 522132. qiongsiwu1 added a comment. Thanks so much @hubert.reinterpretcast !! The comments are addressed. I will land the patch when the pre-commit CI finishes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast accepted this revision. hubert.reinterpretcast added a comment. This revision is now accepted and ready to land. LGTM with minor comments! Comment at: clang/lib/Driver/ToolChains/AIX.cpp:132 +// possible. Then `-bforceimprw` changes such sections to

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 520375. qiongsiwu1 added a comment. Rebase. Ping for review @hubert.reinterpretcast . Thanks so much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files:

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. @hubert.reinterpretcast please let me know if there are comments/suggestions! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review. Specifically, the option is renamed to `-mxcoff-roptr` and `-mno-xcoff-roptr` to indicate that this option only affects the `XCOFF` binary format, so it is expected to not affect `ELF` or other formats. Thanks! Repository: rG LLVM Github

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:37 +// LINK: "-bforceimprw" +// LTO_ROPTR: "-bplugin_opt:-mxcoff-roptr" +// NO_ROPTR-NOT: "-mxcoff-roptr" qiongsiwu1 wrote: > hubert.reinterpretcast wrote: > > This needs the backend

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 marked an inline comment as done. qiongsiwu1 added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:37 +// LINK: "-bforceimprw" +// LTO_ROPTR: "-bplugin_opt:-mxcoff-roptr" +// NO_ROPTR-NOT: "-mxcoff-roptr" hubert.reinterpretcast wrote: >

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 509367. qiongsiwu1 marked an inline comment as done. qiongsiwu1 added a comment. Fix the LTO data sections check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 509360. qiongsiwu1 added a comment. Moving the `-shared` check to the AIX linker and modifying the tests so we test pure linking. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 509342. qiongsiwu1 added a comment. Rebase and address feedback other than test cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files:

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/lib/Driver/ToolChains/AIX.cpp:125 + // The `-mroptr` option places constants in RO sections as much as possible. + // Then `-bforceimprw` changes such sections to RW if they contain imported Old

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/docs/ReleaseNotes.rst:317 + ``-fno-data-sections``. When ``-mxcoff-roptr`` is in effect at link time, + read-only data sections with relocatable address values that resolve to + imported symbols are made

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:647 +def err_roptr_requires_data_sections: Error<"-mxcoff-roptr is supported only with -fdata-sections">; +def err_roptr_cannot_build_shared: Error<"-mxcoff-roptr is not

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. The new option is renamed to `-mxcoff-roptr` to avoid confusions if we are using it during a pure linking job on non-AIX platforms. The option name now implies that it applies to XCOFF only, so silently ignoring the option on other platforms is not that serious a

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 509118. qiongsiwu1 edited the summary of this revision. qiongsiwu1 added a comment. Renaming the `-mroptr` option to `-mxcoff-roptr`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 509091. qiongsiwu1 added a comment. Simplify LTO data section error check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files: clang/docs/ReleaseNotes.rst

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 509057. qiongsiwu1 added a comment. Fixing LTO data sections error check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files: clang/docs/ReleaseNotes.rst

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:714-716 +if (!Args.hasFlag(options::OPT_fdata_sections, + options::OPT_fno_data_sections, UseSeparateSections) && +

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:714-716 +if (!Args.hasFlag(options::OPT_fdata_sections, + options::OPT_fno_data_sections, UseSeparateSections) && +

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 507752. qiongsiwu1 added a comment. Address code review. Thanks for your comments/suggestions @MaskRay !! - Fixing comments. - Using `--target=` in tests. - Reducing number of RUN lines in tests. @hubert.reinterpretcast I ended up having a mix of tests

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:21 +// RUN: FileCheck %s --check-prefix=ROPTR +// RUN: %clang -### -target powerpc64-ibm-aix-xcoff -S -mroptr %s 2>&1 | \ +// RUN: FileCheck %s --check-prefix=ROPTR MaskRay

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5250 + if (Args.hasArg(options::OPT_mroptr) || Args.hasArg(options::OPT_mno_roptr)) { +bool HasRoptr = qiongsiwu1 wrote: > hubert.reinterpretcast wrote: > >

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

MaskRay added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:1 +// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr %s 2>&1 | \ +// RUN: FileCheck %s --check-prefixes=ROPTR,LINK Prefer `--target=` to `-target ` for new tests.

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/AIX.cpp:125 + // `-mroptr` implies the `-bforceimprw` linker option. + // The `-mroptr` option places constants in RO sections as much as possible. Delete `// `-mroptr` implies the

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review/comments. Thanks so much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review/comments. Thanks so much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5250 + if (Args.hasArg(options::OPT_mroptr) || Args.hasArg(options::OPT_mno_roptr)) { +bool HasRoptr = hubert.reinterpretcast wrote: > This only checks for `-m[no-]roptr`

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 505181. qiongsiwu1 added a comment. Thanks for the comments @stephenpeckham ! This update addresses all of them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

stephenpeckham added inline comments. Comment at: clang/docs/ReleaseNotes.rst:230 +- Introduced the ``-mroptr`` option to place constant objects with relocatable + address values in the read-only data section. This option is intended to + be used with the ``-fdata-sections``

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503946. qiongsiwu1 added a comment. Adding LTO error check tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files: clang/docs/ReleaseNotes.rst

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503944. qiongsiwu1 added a comment. Adding tests to check that the default behaviour is identical with `-mno-roptr`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503941. qiongsiwu1 added a comment. Fixing release note. Adding pure linking tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files:

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503934. qiongsiwu1 added a comment. Adding logic to pass `mroptr` to the backend during LTO codegen. Error check is not ideal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503796. qiongsiwu1 added a comment. Rename the `ReadOnlyPointers` option in the source to `XCOFFReadOnlyPointers`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:10 +// ROPTR-NOT: "-mroptr" +// ROPTR-NOT: "-bforceimprw" + hubert.reinterpretcast wrote: > w2yehia wrote: > > hubert.reinterpretcast wrote: > > > Do we pass the option

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5250 + if (Args.hasArg(options::OPT_mroptr) || Args.hasArg(options::OPT_mno_roptr)) { +bool HasRoptr = This only checks for `-m[no-]roptr` when the front-end

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/docs/ReleaseNotes.rst:231 + address values in the read-only data section. This option is intended to + be used with the ``-fdata-sections`` option. When ``-mroptr`` is in effect, + read-only data sections with

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:10 +// ROPTR-NOT: "-mroptr" +// ROPTR-NOT: "-bforceimprw" + w2yehia wrote: > hubert.reinterpretcast wrote: > > Do we pass the option through to the LTO codegen when

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503440. qiongsiwu1 added a comment. Update `-mroptr` cc1 option help text. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files: clang/docs/ReleaseNotes.rst

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 503437. qiongsiwu1 added a comment. Addressing review comments - fixing typos, and updating the tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files:

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:1 +// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr %s 2>&1 | FileCheck %s +// RUN: %clang -### -target powerpc-ibm-aix-xcoff -mroptr -mno-roptr %s 2>&1 | \

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

w2yehia added inline comments. Comment at: clang/test/Driver/ppc-roptr.c:10 +// ROPTR-NOT: "-mroptr" +// ROPTR-NOT: "-bforceimprw" + hubert.reinterpretcast wrote: > Do we pass the option through to the LTO codegen when `-fprofile-generate` is > used? > We may

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added a subscriber: w2yehia. hubert.reinterpretcast added inline comments. Comment at: clang/test/CodeGen/PowerPC/aix-roptr.c:9-10 +// RUN: -S <%s 2>&1 | FileCheck %s --check-prefix=DATA_SECTION_ERR +// RUN: not %clang_cc1

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast added inline comments. Comment at: clang/docs/ReleaseNotes.rst:229-231 +- Introduced the ``-mroptr`` option to place constant objects with relocatable + address values in the ready-only data section. This option is intended to + be used with the

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Ping for review. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 501871. qiongsiwu1 added a comment. Updating `-fdata-sections` related comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files:

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Gentle ping for review. @hubert.reinterpretcast Thanks so much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 501264. qiongsiwu1 added a comment. Update lit test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files: clang/docs/ReleaseNotes.rst

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 501211. qiongsiwu1 added a comment. Updating the release note and adding comments on the `-fdata-sections` requirement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Gentle ping for review. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 500298. qiongsiwu1 added a comment. > This accepts -mno-roptr for other platforms despite having no semantic > functionality (e.g., it does not move variables to a different section for > ELF codegen). Thanks for catching this @hubert.reinterpretcast !!

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

hubert.reinterpretcast requested changes to this revision. hubert.reinterpretcast added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5251 + if (Args.hasFlag(options::OPT_mroptr, options::OPT_mno_roptr,

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

stephenpeckham accepted this revision. stephenpeckham added a comment. This revision is now accepted and ready to land. I don't have issues with this code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 added a comment. Gentle ping for review. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 ___ cfe-commits mailing list

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 updated this revision to Diff 498118. qiongsiwu1 added a comment. Rebase to resolve clang release note conflict. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144190/new/ https://reviews.llvm.org/D144190 Files:

[PATCH] D144190: [AIX][clang] Storage Locations for Constant Pointers

qiongsiwu1 created this revision. qiongsiwu1 added a project: clang. Herald added subscribers: ormris, kbarton, nemanjai. Herald added a project: All. qiongsiwu1 requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. This patch adds clang options `-mroptr` and