This is an archive of the discontinued LLVM Phabricator instance.

[clang] Avoid duplicating ProgramAddressSpace in TargetInfo. NFCI
ClosedPublic

Authored by arichardson on Nov 18 2022, 7:19 AM.

Details

Summary

This value was added to clang/Basic in D111566, but is only used during
codegen, where we can use the LLVM IR DataLayout instead. I noticed this
because the downstream CHERI targets would have to also set this value
for AArch64/RISC-V/MIPS. Instead of duplicating more information between
LLVM IR and Clang, this patch moves getTargetAddressSpace(QualType T) to
CodeGenTypes, where we can consult the DataLayout.

Diff Detail

Event Timeline

arichardson created this revision.Nov 18 2022, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 7:19 AM
arichardson requested review of this revision.Nov 18 2022, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 7:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

clang-format

arichardson added a reviewer: Restricted Project.Nov 18 2022, 7:39 AM

Functionally this looks ok to me. However I am not sure if CodeGenTypes is the 'right' place for this function to live, considering that other functions with similar functionality are in ASTContext - including overloads of getTargetAddressSpace( ). @erichkeane @aaron.ballman could you please take a look?

Functionally this looks ok to me. However I am not sure if CodeGenTypes is the 'right' place for this function to live, considering that other functions with similar functionality are in ASTContext - including overloads of getTargetAddressSpace( ). @erichkeane @aaron.ballman could you please take a look?

My view is that the parts that interact with LLVM IR should really live in CodeGen/ and not Basic/ or AST/. I will see how difficult it would be to move the remaining target (LLVM IR) address space handling code to CodeGen/

Functionally this looks ok to me. However I am not sure if CodeGenTypes is the 'right' place for this function to live, considering that other functions with similar functionality are in ASTContext - including overloads of getTargetAddressSpace( ). @erichkeane @aaron.ballman could you please take a look?

My view is that the parts that interact with LLVM IR should really live in CodeGen/ and not Basic/ or AST/. I will see how difficult it would be to move the remaining target (LLVM IR) address space handling code to CodeGen/

Yeah, I don't think there's a good reason for some of the address-space stuff that currently lives in Basic to be there instead of in CodeGen. Basic/AST need to understand what address spaces exist, their sizes, and what relationships they have with each other, and that's it.

Functionally this looks ok to me. However I am not sure if CodeGenTypes is the 'right' place for this function to live, considering that other functions with similar functionality are in ASTContext - including overloads of getTargetAddressSpace( ). @erichkeane @aaron.ballman could you please take a look?

My view is that the parts that interact with LLVM IR should really live in CodeGen/ and not Basic/ or AST/. I will see how difficult it would be to move the remaining target (LLVM IR) address space handling code to CodeGen/

Yeah, I don't think there's a good reason for some of the address-space stuff that currently lives in Basic to be there instead of in CodeGen. Basic/AST need to understand what address spaces exist, their sizes, and what relationships they have with each other, and that's it.

Thanks for looking at this - does this mean you are happy for me to commit this change?

rjmccall accepted this revision.Dec 1 2022, 10:29 AM

I think this is fine. Most of the patch is changing calls to getTargetAddressSpace to be internal to IRGen, which, as mentioned, I think is the right move.

I do think that if we're going to support multiple program address spaces (which seems to be a goal) that we'll eventually want an AST-level concept of the default program address space, but perpetuating the use of target ASes at the AST level isn't the right way to approach that, so this is still the right first step.

This revision is now accepted and ready to land.Dec 1 2022, 10:29 AM