[CIR] Upstream support for address space handling

[andykaylor]

Overview

Upstream support for correctly handling address spaces in CIR.

This is a pervasive task, requiring changes in many places. Locations where address space handling is known to be missing are marked with a cir::MissingFeatures::addressSpace() assertion. Considerable effort will be needed to verify that all needed handling is reached by some test case. Many cases will not be reachable until support for additional features such as OpenMP or CUDA are added, which may not happen until much later. The assertions should be left in place for such cases.

Suggested minimal test case

void f(int __attribute__((address_space(1))) *arg) {
  return;
}

Existing incubator tests

clang/test/CIR/CodeGen/address-space.c
clang/test/CIR/CodeGen/address-space-conversion.cpp

[andykaylor]

@RiverDave Would you be interested in looking into this?

[RiverDave]

Thanks for reaching out! This looks pretty interesting, you can definitely count on me for this.
I do have some really basic questions regarding the upstreaming process though:

1. I suppose most of the addrspace handling is implemented on the incubator therefore we need to bring those changes where supported upstream?

2. Also, are there any existing conventions or examples in upstream Clang/LLVM you’d suggest I follow for incremental upstreaming (e.g., how tests should be structured, how granular patches should be)? (I believe Amr had worked heavily on upstreaming so perhaps that's a reference?)

[andykaylor]

• I suppose most of the addrspace handling is implemented on the incubator therefore we need to bring those changes where supported upstream?

There are some missing pieces in the incubator, which are marked by assert(!cir::MissingFeatures::addressSpace()); there, but the basic framework for handling address spaces is there, beginning with the cir::AddressSpaceAttr attribute, which can be set on cir::GlobalOp and cir::PointerType.

I'd suggest starting by upstreaming the things that are already done in the incubator, and then if you're feeling comfortable with it you can move on to filling in the pieces that are missing in the incubator.

• Also, are there any existing conventions or examples in upstream Clang/LLVM you’d suggest I follow for incremental upstreaming (e.g., how tests should be structured, how granular patches should be)? (I believe Amr had worked heavily on upstreaming so perhaps that's a reference?)

Yeah. I should probably put together some kind of "introduction to CIR upstreaming" documentation. The short answer is that you'll need to update the incubator code to conform to the style and coding standard expectations of the upstream codebase. The most common things you'll need to change are:

1. Replacing uses of auto which are much less liberal in clang than MLIR code in general and the CIR incubator in particular. The coding standard for use of auto in clang can be found here (and, BTW, we're trying to follow this in new incubator code). Basically, clang only uses auto for things that are extremely verbose (like iterator types) or obvious (like the result of a dyn_cast).

2. Update variable and function names. This is something that evolved as the incubator code was implemented, so it's very inconsistent there. Unfortunately, the standard we ended up with is a bit inconsistent because clang and MLIR follow different guidelines. The expectations are documented here (and, again, new incubator code should follow this). If you use clangd, there are configuration files in place to help.

Most of us who work on upstreaming received a lot of corrections on the points above when we were getting started, so don't feel bad if that happens. Do your best, and reviewers will be happy to help.

[RiverDave]

• I suppose most of the addrspace handling is implemented on the incubator therefore we need to bring those changes where supported upstream?

There are some missing pieces in the incubator, which are marked by assert(!cir::MissingFeatures::addressSpace()); there, but the basic framework for handling address spaces is there, beginning with the cir::AddressSpaceAttr attribute, which can be set on cir::GlobalOp and cir::PointerType.

I'd suggest starting by upstreaming the things that are already done in the incubator, and then if you're feeling comfortable with it you can move on to filling in the pieces that are missing in the incubator.

• Also, are there any existing conventions or examples in upstream Clang/LLVM you’d suggest I follow for incremental upstreaming (e.g., how tests should be structured, how granular patches should be)? (I believe Amr had worked heavily on upstreaming so perhaps that's a reference?)

Yeah. I should probably put together some kind of "introduction to CIR upstreaming" documentation. The short answer is that you'll need to update the incubator code to conform to the style and coding standard expectations of the upstream codebase. The most common things you'll need to change are:

1. Replacing uses of auto which are much less liberal in clang than MLIR code in general and the CIR incubator in particular. The coding standard for use of auto in clang can be found here (and, BTW, we're trying to follow this in new incubator code). Basically, clang only uses auto for things that are extremely verbose (like iterator types) or obvious (like the result of a dyn_cast).
2. Update variable and function names. This is something that evolved as the incubator code was implemented, so it's very inconsistent there. Unfortunately, the standard we ended up with is a bit inconsistent because clang and MLIR follow different guidelines. The expectations are documented here (and, again, new incubator code should follow this). If you use clangd, there are configuration files in place to help.

Most of us who work on upstreaming received a lot of corrections on the points above when we were getting started, so don't feel bad if that happens. Do your best, and reviewers will be happy to help.

Thanks for clarifying. I'm very excited to start working on this! I'll do my best