[PATCH] D144189: [AIX][CodeGen] Storage Locations for Constant Pointers

Wed Feb 22 08:11:24 PST 2023

qiongsiwu1 added a comment.

In D144189#4139426 <https://reviews.llvm.org/D144189#4139426>, @myhsu wrote:

> In D144189#4134864 <https://reviews.llvm.org/D144189#4134864>, @qiongsiwu1 wrote:
>
>> In D144189#4132940 <https://reviews.llvm.org/D144189#4132940>, @myhsu wrote:
>>
>>> 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. 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!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144189/new/

https://reviews.llvm.org/D144189