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

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


On 11/25/21 2:50 AM, Sankisa, Krishna (Chaitanya) via llvm-dev
wrote:
>
> [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*
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.
>
> ;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
>
> Review link: https://reviews.llvm.org/D114533 
> <https://reviews.llvm.org/D114533>
> ⚙ D114533 LLVM IR should allow bitcast between address spaces with the 
> same size. <https://reviews.llvm.org/D114533>
> LLVM IR should allow bitcast between address spaces with the same size.
> reviews.llvm.org
>
>
> Regards,
> Chaitanya
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211129/90a82c43/attachment.html>

> On Nov 29, 2021, at 12:54, Philip Reames via llvm-dev <llvm-dev at
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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211129/2293c237/attachment.html>