This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Add AAAddressSpace to deduce address spaces
ClosedPublic

Authored by tianshilei1992 on Feb 25 2022, 12:33 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jhuber6 created this revision.Feb 25 2022, 12:33 PM
jhuber6 requested review of this revision.Feb 25 2022, 12:33 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert added inline comments.Feb 25 2022, 2:30 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9910

hasXXX sounds like a boolean return, this would be getAS

9917

don't make it required, make the dependence type NONE

9924

Check the return type of this function.

9931

That doesn't work. We cannot ignore things with the "wrong type".
V should never be null, undef can be ignored, a "zero" too if nullptr is not valid, everything else not I think.
See some other uses of getAssumedUnderlyingObjects.

9938

break

9957

before, not after.

9982

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

This indicates a problem, though, not necessarily in your code.

jhuber6 updated this revision to Diff 411528.Feb 25 2022, 2:53 PM

Addressing some comments.

ormris removed a subscriber: ormris.May 16 2022, 11:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 11:22 AM
tianshilei1992 commandeered this revision.Jun 27 2023, 6:34 PM
tianshilei1992 added a reviewer: jhuber6.

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
1514

This is not a good idea.

4870

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
9923

This is now AAUnderlyingValues

9965–9983

We can just replace all uses with the casted pointer, no need for pretty much all of this.

rebase and fix tests

tianshilei1992 added inline comments.Jul 2 2023, 7:13 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10180

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.

jdoerfert added inline comments.Jul 3 2023, 9:48 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
4937

no null opt anymore

4955

docs

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10192–10200

All of this happens implicitly. No need for you to check it. It would also not be required.

10204–10206

Check out other uses of AAUnderlyingObjects. Instead of requiring it, you can just look at the associated value in case the forallUnderlyingObjects call fails.

10214

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.

10241

No need for the vector, change the uses in the first loop.

10247–10250

No need. Just let the attributor change all uses.

jdoerfert added inline comments.Jul 3 2023, 9:48 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10276

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.

arsenm added a subscriber: arsenm.Jul 3 2023, 10:05 AM
arsenm added inline comments.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10259–10261

Doesn't consider vector of pointers

10271

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

tianshilei1992 marked 21 inline comments as done.

address some comments

I'll rewrite the test.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10241

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.

tschuett added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
4937

Could you instead return an optional to handle invalid and unknown states?

jdoerfert added inline comments.Jul 4 2023, 11:57 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10241

It's more complex. More complex is bad.

rebase and fix tests

tianshilei1992 marked 7 inline comments as done.Jul 5 2023, 10:17 PM
tianshilei1992 added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
4937

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.

jdoerfert added inline comments.Jul 6 2023, 3:14 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
4933

Checkout isValidIRPositionForInit in upstream attributes. Use it to restrict this one.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10188

No need for AAIsDead.

10194–10197

No need. Most attributes behave correct w/o state check. This one does too.

10205–10211

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)

10227

Technically, you can replace the value with poison if you have NoAS at the end.

10236–10237

Why check this? What can check is if the pointer is actually an AS cast we can just "undo".

10317

or put this into the initialize of all the ones we do not actually support.

arsenm added inline comments.Jul 6 2023, 3:41 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10266

address spaces only use 24 bits so there's no overflow concern with just u32

tianshilei1992 marked 8 inline comments as done.

rebase, fix comments, fix tests

there should be one test left

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10236–10237

Because the doc of the function checkForAllUses says:

Check \p Pred on all (transitive) uses of \p V.

We only want direct uses of the value here, so we check if the use is the value itself. Did I get it wrong?

10259–10261

Hmm, vector of pointers or pointer of vector?

remove unnecessary changes

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
10230

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.

10236–10237

We only want direct uses of the value here, so we check if the use is the value itself. Did I get it wrong?

We are generally happy modifying any use, I'd say. Should not matter much right now anyway.

10257

Why do we cast this? Keep one type consistently.

tianshilei1992 marked 5 inline comments as done.

rebase, fix tests, and rename to AAAddressSpace

tianshilei1992 retitled this revision from [Attributor] Add AAAddressSpaceInfo to deduce address spaces to [Attributor] Add AAAddressSpace to deduce address spaces.Jul 9 2023, 7:27 PM
tianshilei1992 edited the summary of this revision. (Show Details)
tianshilei1992 added inline comments.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10257

getPointerAddressSpace() returns uint32_t but AssumedAddressSpace is int32_t since AssumedAddressSpace can be -1. There must be a cast.

tianshilei1992 added inline comments.Jul 9 2023, 7:28 PM
llvm/test/Transforms/Attributor/address_space_info.ll
26

This is supposed to be replaced with an addrspacecast.

llvm/test/Transforms/Attributor/value-simplify.ll
788

This should be replaced by @ConstAS3Ptr directly.

jdoerfert added inline comments.Jul 9 2023, 7:30 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
883

remove unrelated changes please.

rebase to remove unrelated changes

tianshilei1992 marked an inline comment as done and 2 inline comments as not done.Jul 9 2023, 7:40 PM
jdoerfert added inline comments.Jul 10 2023, 10:39 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10193
10224–10225

Hoist this.

10251–10252

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.

10280–10281
llvm/test/Transforms/Attributor/address_space_info.ll
10

remove the compiler used part

109
llvm/test/Transforms/Attributor/value-simplify.ll
788

Did you update the test, above it seems to work.

tianshilei1992 marked 6 inline comments as done.

rebase, fix comments, fix tests

the patch is in a good shape now

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10251–10252

Why does the AS of value operand matter here?

llvm/test/Transforms/Attributor/value-simplify.ll
788

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.

jdoerfert accepted this revision.Jul 10 2023, 10:17 PM

LG, assuming the stuff below works out.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10228

early exit.

10251–10252

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
17

Can you undo this for now.

788

Oh, right. Because it is simplified. Leave it for now like this.

This revision is now accepted and ready to land.Jul 10 2023, 10:17 PM
tianshilei1992 marked 4 inline comments as done.

rebase and fix comments

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
10251–10252

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
}
jdoerfert added inline comments.Jul 11 2023, 10:43 PM
llvm/test/Transforms/Attributor/address_space_info.ll
6
28

Now we can see if it works.

rebase and fix comments

This revision was landed with ongoing or failed builds.Jul 12 2023, 12:47 PM
This revision was automatically updated to reflect the committed changes.
arsenm added inline comments.Jul 12 2023, 12:50 PM
llvm/test/Transforms/Attributor/address_space_info.ll
3

How is this doing anything without a target?

74

Use named values

hliao added a subscriber: hliao.Jul 12 2023, 12:51 PM
llvm/test/Transforms/Attributor/address_space_info.ll
3

We have many IR tests w/o a target.

74

K, to avoid potential false alarm in the test? I can do it in a following patch.