[PATCH] D88995: Support vectors in CastInst::isBitOrNoopPointerCastable

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/load.ll:389
+; CHECK-NEXT:    [[X:%.*]] = load <2 x i64>, <2 x i64>* [[P:%.*]], align 16
+; CHECK-NEXT:    [[Y_CAST:%.*]] = inttoptr <2 x i64> [[X]] to <2 x i8*>
+; CHECK-NEXT:    call void @use.v2.p0(<2 x i8*> [[Y_CAST]])
----------------
nlopes wrote:
> lebedev.ri wrote:
> > reames wrote:
> > > lebedev.ri wrote:
> > > > This is going in the opposite direction than what we've just recently disscussed/estabilished - we can't/shouldn't introduce int<->ptr casts that weren't in the source code.
> > > You need to give a lot more context here.  This is simple load forwarding - as done by e.g. GVN.  If you want to change direction, I think that should be separated from this patch.
> > Right. It's D88860 for documentation change, and  D88842 / D88789 / D88788 for some lengthy discussions.
> @reames this is not "simple" load forwarding. This is doing type punning.
> For this transformation to be correct the alias analysis algorithm would need to take all integer stores as potential escape sites. And it doesn't.
> We have two options here: either we change the alias analysis algorithm to take non-ptr memory operations into account and thus make it way more conservative, or we disallow implicit int<->ptr casts done through memory operations (such that AA doesn't need to care about non-ptr stores).
> Since int<->ptr casts are not that frequent, I think it's better to go with the latter option and keep alias analysis as aggressive as we can for the common case.
> 
> If we agree on the statement above, this means this patch is not correct. Yes, LLVM is broken in other places, but at least let's not make it more broken that what it already is. I appreciate the effort to make the vector ops equivalent to the scalar ops, but right now it's not useful to do this as we need to fix the scalar ops first.
> Since int<->ptr casts are not that frequent, I think it's better to go with the latter option and keep alias analysis as aggressive as we can for the common case.

I'm actually tracking towards that with D88789 and now D88979.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88995/new/

https://reviews.llvm.org/D88995