Page MenuHomePhabricator

LLVM IR should allow bitcast between address spaces with the same size.
Needs ReviewPublic

Authored by skc7 on Wed, Nov 24, 5:58 AM.

Details

Reviewers
sameerds
arsenm
Summary

When the addrspacecast instruction was added, the ability to bitcast between pointers from different address spaces was removed.
There are cases, where after analysis, cast between pointers from different address spaces can be concluded to be a no-op cast.
If bitcast can be allowed in these scenarios, it would help further optimise the IR in Transform passes since its a no-op cast.
This enhancement to bitcast will require that pointers to the two address spaces have the same bit widths(can be queried from DataLayout).
Frontend should never misuse the bitcast wherever addrspace cast would have been more appropriate.

For example consider the below IR:

GVN pass has discovered a reinterpretation of bits via a store followed by a load.

%struct.S.coerce = type { i32 addrspace(1)* }
%s.sroa.0 = alloca i32*, align 8, addrspace(5)
%0 = extractvalue %struct.S.coerce %s.coerce, 0
%1 = bitcast i32* addrspace(5)* %s.sroa.0 to i32 addrspace(1)* addrspace(5)*
%2 = addrspacecast i32 addrspace(1)* addrspace(5)* %1 to i32 addrspace(1)
store i32 addrspace(1)* %0, i32 addrspace(1)
%2, align 8

%3 = load i32*, i32* addrspace(5)* %s.sroa.0, align 8, !tbaa !2

GVN pass currently introduces no-op ptrotoint/inttoptr for load.
%3 = ptrtoint i32 addrspace(1)* %0 to i64
%4 = inttoptr i64 %3 to i32*

If bitcast of pointers from different address is allowed, load can be replaced with no-op bitcast
%3 = bitcast i32 addrspace(1)* %0 to i32*

Diff Detail

Event Timeline

skc7 created this revision.Wed, Nov 24, 5:58 AM
skc7 requested review of this revision.Wed, Nov 24, 5:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Nov 24, 5:58 AM

Is there an RFC? This quite seems like asking for problems.

Patch description should include this avoids a need to introduce ptrtoint/inttoptr pairs

skc7 edited the summary of this revision. (Show Details)Wed, Nov 24, 6:59 AM

Patch description should include this avoids a need to introduce ptrtoint/inttoptr pairs

That is a good point, but all the motivational cases seem to be about chaining the address space of the pointee pointer.
Does this come up for changing the address space of the pointer itself? I'm just wondering if this is still relevant with opaque pointers.

Patch description should include this avoids a need to introduce ptrtoint/inttoptr pairs

That is a good point, but all the motivational cases seem to be about chaining the address space of the pointee pointer.
Does this come up for changing the address space of the pointer itself? I'm just wondering if this is still relevant with opaque pointers.

Nothing about address spaces changes with opaque pointers. The address space is a property of the pointer itself, not the memory it points to. This is for dealing with cases where we end up with a bit reinterpret of a pointer in another address space, and need to do a true no-op cast to convert them.

jrtc27 added a subscriber: jrtc27.Thu, Nov 25, 7:24 AM

This seems like it should not apply to non-integral address spaces?

llvm/docs/LangRef.rst
11005

This should be illegal, not legal and undefined, you really want the IR verifier to be able to pick up on this at least, if not an assertion at creation time

llvm/include/llvm/IR/InstrTypes.h
729

Typo "e"

This seems like it should not apply to non-integral address spaces?

No, it shouldn’t care about the individual address spaces. It’s a reinterpret of the bits, the type meanings don’t matter

This seems like it should not apply to non-integral address spaces?

No, it shouldn’t care about the individual address spaces. It’s a reinterpret of the bits, the type meanings don’t matter

Well, they absolutely do for us downstream, but we also don't use the non-integral address space feature, since it didn't exist when our fork started and also has overly restrictive semantics. Normally the places we need to stop these kinds of casts/reinterpretations are a subset of those for non-integral address spaces, but maybe this is one of those exceptions then. I certainly have no clue about what the non-integral address spaces are actually used for and mean :)

skc7 updated this revision to Diff 389983.Fri, Nov 26, 3:55 AM

Updated diff with changes suggested by jrtc27