[AMD Official Use Only]
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
[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.
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.
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.
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`.
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.
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
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.
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.
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.
[AMD Official Use Only]