[llvm-dev] [RFC] : LLVM IR should allow bitcast between address spaces with the same size

11 views
Skip to first unread message

Sankisa, Krishna (Chaitanya) via llvm-dev

unread,
Nov 25, 2021, 5:50:20 AM11/25/21
to llvm...@lists.llvm.org

[AMD Official Use Only]


TL;DR
=====

We propose the following change to LLVM IR:
- Allow bitcast to support no-op pointer cast between pointers from different address spaces.
- This bitcast is valid if the bit widths queried for addressspaces from datalayout match.
- Overload CastIsValid call with datalayout argument to check validity of cast.
- Update CastIsValid to allow bitcast between vector of pointers from different address spaces if total bit widths match.
- GVN pass introduces ptrtoint/inttoptr for load which reinterprets bits from previous store.
  Instead use a no-op bitcast of ptrs from different address spaces.


Motivation
==========

When addrspacecast was introduced, abilty to do no-op pointer bitcast from different address spaces has been removed.
Pointer sizes are always known from DataLayout which is now made mandatory in LLVM IR.
So, Bitcast can be analysed to be no-op cast by matching the pointer sizes from DataLayout.

Since there is no other way to do no-op reinterpret of bits, in some cases GVN pass introduces a ptrtoint/inttoptr pair.
After proper analysis, that a no-op bitcast can be done is concluded, then a bitcast can be introduced.
Usage of no-op pointer bitcast between addrspaces can be restricted to be used only by IR Transform passes but not by frontend.

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*


Implementation
==============

1. There are certain cases where standalone instructions are created without linking them to basicblock/function or module.
   In such cases DataLayout is not accessible by querying the module. To check validity of bitcast datalayout is mandatory.
   So CastInst::CastIsValid, CastInst::create etc have been overloaded to take DataLayout as argument.

   static bool castIsValid(Instruction::CastOps op, Value *S, Type *DstTy,
                           const DataLayout &DL);

   static CastInst *Create(
      Instruction::CastOps,   ///< The opcode of the cast instruction
      Value *S,               ///< The value to be casted (operand 0)
      Type *Ty,               ///< The type to which cast should be made
      const DataLayout &DL,   ///< DataLayout to check validity of bitcast
      const Twine &Name = "", ///< Name for the instruction
      Instruction *InsertBefore = nullptr ///< Place to insert the instruction
   );

2. Verifier has been updated to check for validity of bitcast using datalayout.

3. GVN pass has been updated to use bitcast for a load instead of emitting ptrtoint/ inttoptr.
   llvm/lib/Transforms/Utils/VNCoercion.cpp

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

Regards,
Chaitanya

Serge Pavlov via llvm-dev

unread,
Nov 25, 2021, 1:10:58 PM11/25/21
to Sankisa, Krishna (Chaitanya), llvm...@lists.llvm.org
There are targets, where address spaces are separate, that is the same value addresses different entities in different spaces. Size of the pointers in these spaces can be the same. Cast between such pointers is invalid operation.

Thanks,
--Serge


_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Sankisa, Krishna (Chaitanya) via llvm-dev

unread,
Nov 29, 2021, 7:49:56 AM11/29/21
to Serge Pavlov, llvm...@lists.llvm.org

[AMD Official Use Only]



This bitcast is intended to be used by transform passes when its been decided that the resultant operation is no-op( Like in GVN case of inserting ptrtoint/inttoptr ).
When reinterpretation of bits is not supported by target, the resultant value is poison.

Regards,
Chaitanya.

From: Serge Pavlov <sepa...@gmail.com>
Sent: 25 November 2021 23:40
To: Sankisa, Krishna (Chaitanya) <Krishna...@amd.com>
Cc: llvm...@lists.llvm.org <llvm...@lists.llvm.org>
Subject: Re: [llvm-dev] [RFC] : LLVM IR should allow bitcast between address spaces with the same size
 
[CAUTION: External Email]

Renato Golin via llvm-dev

unread,
Nov 29, 2021, 8:08:00 AM11/29/21
to Sankisa, Krishna (Chaitanya), llvm...@lists.llvm.org
On Mon, 29 Nov 2021 at 12:50, Sankisa, Krishna (Chaitanya) via llvm-dev <llvm...@lists.llvm.org> wrote:

This bitcast is intended to be used by transform passes when its been decided that the resultant operation is no-op( Like in GVN case of inserting ptrtoint/inttoptr ).

When reinterpretation of bits is not supported by target, the resultant value is poison.

This will depend on the target's own interpretation of what each address space is meant to be.

Each optimisation that uses this will have to ask the target info classes for the similarity (for some definition of) between two address spaces and then checking if the cast is actually a no-op or not.

As Serge said, some targets use address spaces for things that are completely segregated even if they have the same data layout (ex. [1]). Bit-casting on those cases is an invalid operation.

As a result, the infrastructure that allows this must be extremely conservative (ie. return false on the base class) and only return true after enough checks are done by the target library, and then it's up to that target to allow/deny bit-casts between address spaces (which at that point, don't even need to have the same data layout).

In a nutshell, I don't think *only* looking at the data layout will be safe. You need to dig deeper and make that a target decision, and allow passes to completely ignore this specific pattern (ie. avoid searching the whole IR for it) if there's no chance it will ever be useful. This makes sure targets that don't support that don't pay the penalty of traversing the IR for no benefit, because the response will always be `false`.

Makes sense?

cheers,
--renato

Philip Reames via llvm-dev

unread,
Nov 29, 2021, 12:55:05 PM11/29/21
to Sankisa, Krishna (Chaitanya), llvm...@lists.llvm.org

Strong -1 here.  This seems like an abuse of bitcast to avoid nailing down interfaces and semantics for restricted address space casts.  IMO, we should not accept the proposed IR change and should instead work on standardizing APIs/semantics for restricted addrspacecasts.  


Philip

Honestly, this really looks like a bug in GVN.  We could reasonable insert an *addrspacecast*, but the round trip through integer values seems highly suspect.


Also, a key missing detail in your example is that you've taught your alias analysis to peak through addrspacecasts.  I don't believe the default implementation will conclude that %s.sroa.0 and %2 are mustalias.  Nothing wrong with it, just noting it due to the potential confusion. 

Matt Arsenault via llvm-dev

unread,
Nov 29, 2021, 1:15:17 PM11/29/21
to Renato Golin, llvm...@lists.llvm.org, Sankisa, Krishna (Chaitanya)

On Nov 29, 2021, at 08:07, Renato Golin via llvm-dev <llvm...@lists.llvm.org> wrote:

On Mon, 29 Nov 2021 at 12:50, Sankisa, Krishna (Chaitanya) via llvm-dev <llvm...@lists.llvm.org> wrote:

This bitcast is intended to be used by transform passes when its been decided that the resultant operation is no-op( Like in GVN case of inserting ptrtoint/inttoptr ).

When reinterpretation of bits is not supported by target, the resultant value is poison.

This will depend on the target's own interpretation of what each address space is meant to be.

No, this change doesn’t really have to do with the address spaces at all. It’s fixing the representation of a no-op cast so the optimizer does not not have to introduce ptrtoint. It isn’t intended as a semantic change for the interpretation of address spaces or address space casts.


Each optimisation that uses this will have to ask the target info classes for the similarity (for some definition of) between two address spaces and then checking if the cast is actually a no-op or not.

The point here is this optimization does not care about whether a proper address space cast is a no-op or not. The original code was not performing an address space cast, it was doing type punning and reinterpreting some bits as a different address space.


As Serge said, some targets use address spaces for things that are completely segregated even if they have the same data layout (ex. [1]). Bit-casting on those cases is an invalid operation.

This is true, but this isn’t directly related to this change. If the target does not have no-op conversions between a given pair of address spaces, the original code was broken to begin with. This change does not allow introducing new illegal address space conversions that were not already present in the original program. It’s garbage in, garbage out. If the target can’t really reinterpret these pointers, it would be UB on access.



As a result, the infrastructure that allows this must be extremely conservative (ie. return false on the base class) and only return true after enough checks are done by the target library, and then it's up to that target to allow/deny bit-casts between address spaces (which at that point, don't even need to have the same data layout).
This would be introducing target dependence on core IR instruction semantics, which would be bad. The transformation this intends to fix would not be improved by target information. This pointer-reinterpret-to-cast transformation is and should be done unconditionally. If we were to introduce target information here, it would look something like if (isNoopCast()) { doNoOpAddrSpaceCast } else { doPtrToIntIntToPtr}. This doesn't avoid the need to introduce ptrtoint, it’s just given some targets better IR some of the time.



In a nutshell, I don't think *only* looking at the data layout will be safe. You need to dig deeper and make that a target decision, and allow passes to completely ignore this specific pattern (ie. avoid searching the whole IR for it) if there's no chance it will ever be useful. This makes sure targets that don't support that don't pay the penalty of traversing the IR for no benefit, because the response will always be `false`.
To reiterate, there is no target specific pattern to search for here. There is a general type punning pattern and we need to introduce new IR that represents the same type punning, only using IR values instead of memory. Currently the only way to represent this is with an inttptr/ptrtoint pair, which everyone agrees the optimizers should never introduce.

-Matt

Matt Arsenault via llvm-dev

unread,
Nov 29, 2021, 1:19:18 PM11/29/21
to Philip Reames, llvm...@lists.llvm.org, Sankisa, Krishna (Chaitanya)

On Nov 29, 2021, at 12:54, Philip Reames via llvm-dev <llvm...@lists.llvm.org> wrote:

Strong -1 here.  This seems like an abuse of bitcast to avoid nailing down interfaces and semantics for restricted address space casts.  IMO, we should not accept the proposed IR change and should instead work on standardizing APIs/semantics for restricted addrspacecasts.  

I think you are misunderstanding the point of the change. The motivating example is specifically not an address space cast. The reason to do this is not to improve the optimization of no-op address space casts, although that is a potential side effect. The point is to give the IR a way to represent a cast we know is a no-op cast to represent type punning in the original program without resorting to ptrtoint/inttoptr. This is to avoid the pessimization of ptrtoint, not to make no-op addrspacecast optimizations better.

-Matt

Philip Reames via llvm-dev

unread,
Nov 29, 2021, 1:28:52 PM11/29/21
to Matt Arsenault, llvm...@lists.llvm.org, Sankisa, Krishna (Chaitanya)

My counter proposal would be: use an addrspacecast for the cast.  Have GVN insert an addrspacecast instead of ptrtoint/inttoptr. 

From GVNs perspective, type punning *implies* that the resulting addrspacecast must be mustalias.  It doesn't have to explicitly query anything to prove that.  One thing to highlight: Two pointers in different addrspaces can be mustalias *without* sharing the same bit pattern.  As a result, the GVN case implies mustalias, but *not* pointer equality. 

(I forget where we stand on whether addrspacecast is allowed to fault.  If we allow faults, we need a semantic restriction on introducing the addrspacecast, but that's simply a target property check.  I vaguely think we don't allow addrspacecast to fault, and thus this aside is irrelevant.)

For the nop case (e.g. where the bits are the same), add a TTI hook, and teach ComputeKnownBits and other analyzes to selectively look back through them.

Philip


Renato Golin via llvm-dev

unread,
Nov 30, 2021, 4:51:47 AM11/30/21
to Matt Arsenault, llvm...@lists.llvm.org, Sankisa, Krishna (Chaitanya)
On Mon, 29 Nov 2021 at 18:15, Matt Arsenault <ars...@gmail.com> wrote:
This is true, but this isn’t directly related to this change. If the target does not have no-op conversions between a given pair of address spaces, the original code was broken to begin with. This change does not allow introducing new illegal address space conversions that were not already present in the original program. It’s garbage in, garbage out. If the target can’t really reinterpret these pointers, it would be UB on access.

Ah, I see.

So IIUC, adding the casts in the first place would have to know about target address spaces, but this specific change only allows eliding the inttoptr cast in the specific case where the address spaces have the same data layout?

Assuming the initial cast was legal to begin with, this looks sensible to me.

I was worried this would allow (encourage?) front-ends and other passes to add an address space cast where none (could have) existed in the first place.

Sorry for the noise.

--renato

Matt Arsenault via llvm-dev

unread,
Nov 30, 2021, 7:21:32 PM11/30/21
to Philip Reames, llvm...@lists.llvm.org, Sankisa, Krishna (Chaitanya)
This still forces adding more target information into places where it does not belong, Additionally, this is still no help if the given pair of address spaces is not a no-op. We would still have to fallback to the ptrtoint/inttoptr pair, so there’s still a semantic hole in the IR that requires introducing them

-Matt

Matt Arsenault via llvm-dev

unread,
Nov 30, 2021, 7:27:13 PM11/30/21
to Renato Golin, llvm...@lists.llvm.org, Sankisa, Krishna (Chaitanya)

On Nov 30, 2021, at 04:51, Renato Golin <reng...@gmail.com> wrote:

On Mon, 29 Nov 2021 at 18:15, Matt Arsenault <ars...@gmail.com> wrote:
This is true, but this isn’t directly related to this change. If the target does not have no-op conversions between a given pair of address spaces, the original code was broken to begin with. This change does not allow introducing new illegal address space conversions that were not already present in the original program. It’s garbage in, garbage out. If the target can’t really reinterpret these pointers, it would be UB on access.

Ah, I see.

So IIUC, adding the casts in the first place would have to know about target address spaces, but this specific change only allows eliding the inttoptr cast in the specific case where the address spaces have the same data layout?

Assuming the initial cast was legal to begin with, this looks sensible to me.
The target’s notion of address space casts doesn’t apply here. The user decided to reinterpret the pointer in a different address space, which may or may not be valid to use on the target.


I was worried this would allow (encourage?) front-ends and other passes to add an address space cast where none (could have) existed in the first place.

No, we’re converting from an in-memory representation of reinterpreting bits to one in values. Frontends still need to have some contextual knowledge of what the language and target expects out of address spaces. 

-Matt

Sankisa, Krishna (Chaitanya) via llvm-dev

unread,
Dec 7, 2021, 1:40:42 AM12/7/21
to llvm...@lists.llvm.org, Matt Arsenault

[AMD Official Use Only]


Re-iterating the proposed change and clarifying the concerns raised :

Proposed change:
- Allow no-op bitcast between pointers of same size but from different addrspaces.

Clarifying the concerns:
- No-op reinterprent of bits between addrspaces is already done in the IR like introducing ptrtoint/inttoptr(ex. GVN).
  Main motivation for this change is to give the IR a way to represent a cast we know is a no-op without resorting to ptrtoint/inttoptr.

- With this change we do not want to introduce new illegal address space conversions that were not already present in the original program.
  The original code was not performing an address space cast, it was doing type punning and reinterpreting some bits as a different address space.
  It was broken to begin with if the target didn't allow no-op conversions between given pair of address spaces. We are not intending to change that.  
  Since the IR transform like in GVN case, did a no-op reinterpretation of bits between addrspaces, We are just trying to optimise it by using a no-op bitcast.

- Since the original IR already did this reinterpret of bits without querying the target, the change we propose will also not query the target.
  This pointer-reinterpret-to-cast transformation is and should be done unconditionally. Querying TTI to check for no-op would introduce target dependence on Core IR instruction semantics.
  We used DataLayout to match the pointer sizes. This check is sufficient to allow the no-op bitcast.

- Also, there is no target specific pattern to search for here. Irrespective of the target, we need to introduce new IR that represents the same no-op reinterpret of bits between addrspaces.
  All the targets are intended to benefit from this change.

Regards,
Chaitanya

From: Matt Arsenault <whatmannerof...@gmail.com> on behalf of Matt Arsenault <ars...@gmail.com>
Sent: 01 December 2021 05:57
To: Renato Golin <reng...@gmail.com>
Cc: Sankisa, Krishna (Chaitanya) <Krishna...@amd.com>; llvm...@lists.llvm.org <llvm...@lists.llvm.org>

Subject: Re: [llvm-dev] [RFC] : LLVM IR should allow bitcast between address spaces with the same size
 
[CAUTION: External Email]

Reply all
Reply to author
Forward
0 new messages