This patch adds initial support for the AAAddressSpace abstract
attributor interface to deduce and query address space information for a
pointer. We simply query the underlying objects that a pointer can point
to and find a common address space if they exist. This is the minimal
support for the interface, we currently manifest changes on loads and
stores. Additionally we should use the target transform information to
deduce if an address space transformation is a no-op for the target
machine when calculating compatibility.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
11738 | hasXXX sounds like a boolean return, this would be getAS | |
11745 | don't make it required, make the dependence type NONE | |
11752 | Check the return type of this function. | |
11759 | That doesn't work. We cannot ignore things with the "wrong type". | |
11766 | break | |
11785 | before, not after. | |
11810 | We should reuse the rewrite capabilities of the existing pass, or at least clone loads/stores properly so we don't loose metadata and such. | |
llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll | ||
233 ↗ | (On Diff #411499) | This indicates a problem, though, not necessarily in your code. |
We can get this in such that we only modify accesses first. However, we need to rewrite other pointers (arguments, phi's, geps, ...) in a follow up as that can save registers.
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
1798 | This is not a good idea. | |
5951 | I don't think we want an inc state. It is really just <no-AS/dead, AS-X, unknown> | |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
11751 | This is now AAUnderlyingValues | |
11793–11811 | We can just replace all uses with the casted pointer, no need for pretty much all of this. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12033 | We probably want an assertion here. Returning 0 is not necessarily correct for the case where the AS associated with the value is non-zero but somehow the state is invalid. Essentially we don't want users to call this method if the AA is invalid. |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
6029 | no null opt anymore | |
6047 | docs | |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
12045–12053 | All of this happens implicitly. No need for you to check it. It would also not be required. | |
12057–12059 | Check out other uses of AAUnderlyingObjects. Instead of requiring it, you can just look at the associated value in case the forallUnderlyingObjects call fails. | |
12067 | Vector is overkill. You can return false as soon as we know we have more than one value. You can directly work on the AssumedAddressSpace. | |
12094 | No need for the vector, change the uses in the first loop. | |
12100–12103 | No need. Just let the attributor change all uses. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12129 | add a statistic counter please | |
llvm/test/Transforms/Attributor/address_space_info.ll | ||
3 | remove the max iterations flag | |
5 | transform this to opaque ptr please. | |
18 | What are we testing here? | |
33 | What are we testing here? | |
197 | Make a test where the value and pointer operand are the same and can be casted to an AS. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12112–12114 | Doesn't consider vector of pointers | |
12124 | Stick to IR addrspace name? | |
llvm/test/Transforms/Attributor/address_space_info.ll | ||
289 | test intrinsics. memcpy, memmove, memset and some target ones. Also the predicates, like llvm.amdgcn.is.shared/is.private |
I'll rewrite the test.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12094 | I think it looks more clean if we first collect all uses to be changed and then create the cast instruction as we only consider two cases here after all. | |
llvm/test/Transforms/Attributor/address_space_info.ll | ||
289 | We only look into two kinds of interactions here. We will not need them *for now*. For sure the following patch(es) can cover cases like the runtime check of AS since we might eliminate them if feasible. |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
6029 | Could you instead return an optional to handle invalid and unknown states? |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12094 | It's more complex. More complex is bad. |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
6029 | I could do it but we can also assume this function should not be called if the AA is invalid, which is what is being used right now. I suppose users have to check the state of the AA before query the AS. | |
llvm/test/Transforms/Attributor/address_space_info.ll | ||
89 | This looks so twisted. Have to fix it. |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
6025 | Checkout isValidIRPositionForInit in upstream attributes. Use it to restrict this one. | |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
12041 | No need for AAIsDead. | |
12047–12050 | No need. Most attributes behave correct w/o state check. This one does too. | |
12058–12064 | Do all of this is takeAS, no need to have the logic split. It should return true if we are still "valid", and false otherwise. You can just do return takeAS(AS) | |
12080 | Technically, you can replace the value with poison if you have NoAS at the end. | |
12089–12090 | Why check this? What can check is if the pointer is actually an AS cast we can just "undo". | |
12170 | or put this into the initialize of all the ones we do not actually support. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12119 | address spaces only use 24 bits so there's no overflow concern with just u32 |
rebase, fix comments, fix tests
there should be one test left
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12089–12090 | Because the doc of the function checkForAllUses says:
We only want direct uses of the value here, so we check if the use is the value itself. Did I get it wrong? | |
12112–12114 | Hmm, vector of pointers or pointer of vector? |
I think this is basically done for a first version. I left last comments. We can improve on this in a follow up
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12083 | For stores you need to check it's the pointer operand, assuming it would be invalid IR to change the value operand with one of a different type. You should make a test and verify if it's ok or not. If it's OK, this is OK. | |
12089–12090 |
We are generally happy modifying any use, I'd say. Should not matter much right now anyway. | |
12110 | Why do we cast this? Keep one type consistently. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12110 | getPointerAddressSpace() returns uint32_t but AssumedAddressSpace is int32_t since AssumedAddressSpace can be -1. There must be a cast. |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
1129 | remove unrelated changes please. |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12046 | ||
12077–12078 | Hoist this. | |
12104–12105 | This doesn't actually help as it does not filter store ptr %p, ptr %p. Again, if we can just change both uses, then let's do so. If the verifier is unhappy, check that the use U is the pointer operand. | |
12133–12134 | ||
llvm/test/Transforms/Attributor/address_space_info.ll | ||
10 | remove the compiler used part | |
109 | ||
llvm/test/Transforms/Attributor/value-simplify.ll | ||
804 | Did you update the test, above it seems to work. |
rebase, fix comments, fix tests
the patch is in a good shape now
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12104–12105 | Why does the AS of value operand matter here? | |
llvm/test/Transforms/Attributor/value-simplify.ll | ||
804 | I expect it to be store i32 0, ptr addrspace(3) @ConstAS3Ptr, align 4 directly but actually here it is more complicated than I thought. |
LG, assuming the stuff below works out.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12081 | early exit. | |
12104–12105 | because the verifier might not like us replacing the value ptr %p with ptr addrspace(3) %q. Make a test with that patter, if it passes, we are A-OK. | |
llvm/test/Transforms/Attributor/value-simplify.ll | ||
18 | Can you undo this for now. | |
804 | Oh, right. Because it is simplified. Leave it for now like this. |
rebase and fix comments
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
12104–12105 | I tested the following case and it works: define internal void @_Z13unknown_writePi(ptr addrspace(1) noundef %p) #0 { entry: store ptr addrspace(1) %p, ptr addrspace(1) %p, align 4 ret void } |
remove unrelated changes please.