Support vectors in CastInst::isBitOrNoopPointerCastable

[reames]

Summary

Add support for vectors to this utility function. As noted in the diffs, this utility is used in several transforms so adding the generic logic gets picked up in several places.

The LangRef wording is rather vague here, but you can see analogous logic in VNCoercion.cpp and the Verifier's rules for inttoptr/ptrtoint.

Note that the isNoopCast function is currently incorrect for vectors. I plan on fixing that in a follow up, and then trying to merge the code paths. (Before this change, isBitOrNoopPointerCastable was merely incomplete, not incorrect.)

[vtjnash]

Makes sense to me

[lebedev.ri]

llvm/test/Transforms/InstCombine/load.ll

389

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.

[reames]

llvm/test/Transforms/InstCombine/load.ll

389

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.

[lebedev.ri]

Let me try to approach this slightly differently..
In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?
If not, then the fact that we ended up with two loads is the problem that needs solving.
Does D88979 help?
If not, would it please be possible to see some kind of an end-to-end test showing how that still happens?

llvm/test/Transforms/InstCombine/load.ll

389

Right. It's D88860 for documentation change, and D88842 / D88789 / D88788 for some lengthy discussions.

[reames]

Roman,

I think you're bringing in a concern to this review which does not belong here. Simple load forwarding is a transformation we implement in multiple locations (GVN, EarlyCSE, InstCombine). This patch doesn't even change how we handle inttoptrs. Existing code today will forward an integer load to a pointer load and insert an inttoptr. All this patch does is be consistent about handling the same cases for vectors. I believe we should separate the concerns, and not commingle them.

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later. If you want to avoid inttoptrs, we could canonicalize this case (consistently across the optimizer, not just here) to loading pointers and casting to ints. I have to admit I don't fully understand the reasoning behind the desire to avoid inttoptr, so I'm not sure if this actually helps you or not.

[lebedev.ri]

In D88995#2323613, @reames wrote:

Roman,

I think you're bringing in a concern to this review which does not belong here.

I personally think it's a question of overall optimization pipeline sanity in light of recent discussions in related patches.
Sure, this isn't broken per-wording, but it is likely mis-directed, and it might be good to evaluate that before making things more entrenched.

In D88995#2323613, @reames wrote:

Simple load forwarding is a transformation we implement in multiple locations (GVN, EarlyCSE, InstCombine). This patch doesn't even change how we handle inttoptrs. Existing code today will forward an integer load to a pointer load and insert an inttoptr. All this patch does is be consistent about handling the same cases for vectors.

Yes.

In D88995#2323613, @reames wrote:

I believe we should separate the concerns, and not commingle them.

In D88995#2323613, @reames wrote:

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later.

No, that was not my question.
My question was:

In D88995#2318600, @lebedev.ri wrote:

In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?

In D88995#2323613, @reames wrote:

If you want to avoid inttoptrs, we could canonicalize this case (consistently across the optimizer, not just here) to loading pointers and casting to ints. I have to admit I don't fully understand the reasoning behind the desire to avoid inttoptr, so I'm not sure if this actually helps you or not.

Unfortunately, that's the caveat, the whole reason i'm asking this is because i've tried that already in D88842.

llvm/test/Transforms/InstCombine/call.ll

238

Please can you autogenerate the checklines here and precommit?

[lebedev.ri]

llvm/lib/IR/Instructions.cpp

3235–3237

I think this is fixed vector specific, so you likely want FixedVectorType.

[reames]

In D88995#2323615, @lebedev.ri wrote:

In D88995#2323613, @reames wrote:

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later.

No, that was not my question.
My question was:

In D88995#2318600, @lebedev.ri wrote:

In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?

Ah, we're talking past each other. There is no "unoptimized IR" that I'm working from. I found this while doing an audit of code for consistent handling of non-integral pointers, nothing more. Given that, I can not answer the question you asked.

Again, I believe this entire topic of irrelevant for this particular review. If you want to discuss further, I'm happy to jump on a call next week and brainstorm, but continuing the conversation on the broader direction you want to move in here is not helpful.

[lebedev.ri]

In D88995#2323636, @reames wrote:

In D88995#2323615, @lebedev.ri wrote:

In D88995#2323613, @reames wrote:

To your actual question - which again, I believe is off topic for this review - we could consider extending the load transform to select the "better" of the two types, and potentially insert a cast for the former set of uses instead of the later.

No, that was not my question.
My question was:

In D88995#2318600, @lebedev.ri wrote:

In the most original unoptimized IR, were there actually two different loads, one of a pointer and one of an integer?

Ah, we're talking past each other. There is no "unoptimized IR" that I'm working from. I found this while doing an audit of code for consistent handling of non-integral pointers, nothing more. Given that, I can not answer the question you asked.

How is it "again" if this is the first time you're stating this?
Apologies if i'm being blind and it was said before.

So IOW this isn't actually *known* to happen in reality,
it's a missed opportunity in the existing fold that was found
solely by looking at the folds, not IR.

Please do actually state that in the patches description,
and please do consider saying things like that beforehand
in next patches..

With that in light i guess this should be fine.

Again, I believe this entire topic of irrelevant for this particular review. If you want to discuss further, I'm happy to jump on a call next week and brainstorm, but continuing the conversation on the broader direction you want to move in here is not helpful.

[nlopes]

llvm/test/Transforms/InstCombine/load.ll

389

@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.

[lebedev.ri]

llvm/test/Transforms/InstCombine/load.ll

389

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.

[lebedev.ri]

I'm just gonna re-block this, since this is likely pretty much the code i will be removing in follow-ups.
Feel free to ignore this though, i won't do tug-of-revert.

[reames]

Abandoning an old review I'm not going to return to any time soon.

llvm/lib/IR/Instructions.cpp

if (SrcBits != DestBits)
     return false;
 
   if (DestTy->isX86_MMXTy() || SrcTy->isX86_MMXTy())
     return false;
 
   return true;
 }
 
-bool CastInst::isBitOrNoopPointerCastable(Type *SrcTy, Type *DestTy,
-                                          const DataLayout &DL) {
+/// Return true if conversion is valid with an inttoptr/ptrtoint.  Note that
+/// the results are expected to match CastInst::isNoopCast, but we can't use
+/// that directly since it doesn't check preconditions.
+static bool isNoopPointerCastable(Type *SrcTy, Type *DestTy,
+
+  if (VectorType *SrcVecTy = dyn_cast<VectorType>(SrcTy)) {
+    if (VectorType *DestVecTy = dyn_cast<VectorType>(DestTy)) {
+      if (SrcVecTy->getElementCount() == DestVecTy->getElementCount()) {
+        // An element by element cast. Valid if casting the elements is valid.
+        SrcTy = SrcVecTy->getElementType();
+        DestTy = DestVecTy->getElementType();
+      }
+    }
+  }
+  
   // ptrtoint and inttoptr are not allowed on non-integral pointers
   if (auto *PtrTy = dyn_cast<PointerType>(SrcTy))
     if (auto *IntTy = dyn_cast<IntegerType>(DestTy))
       return (IntTy->getBitWidth() == DL.getPointerTypeSizeInBits(PtrTy) &&
               !DL.isNonIntegralPointerType(PtrTy));
   if (auto *PtrTy = dyn_cast<PointerType>(DestTy))
     if (auto *IntTy = dyn_cast<IntegerType>(SrcTy))
       return (IntTy->getBitWidth() == DL.getPointerTypeSizeInBits(PtrTy) &&
               !DL.isNonIntegralPointerType(PtrTy));
 
+  return false;
+}
+  
+bool CastInst::isBitOrNoopPointerCastable(Type *SrcTy, Type *DestTy,
+                                          const DataLayout &DL) {
+
+  if (isNoopPointerCastable(SrcTy, DestTy, DL))
+    return true;
   return isBitCastable(SrcTy, DestTy);
 }
 
 // Provide a way to get a "cast" where the cast opcode is inferred from the
 // types and size of the operand. This, basically, is a parallel of the
 // logic in the castIsValid function below.  This axiom should hold:
 //   castIsValid( getCastOpcode(Val, Ty), Val, Ty)
 // should not assert in castIsValid. In other words, this produces a "correct"

llvm/test/Transforms/InstCombine/call.ll

 }
 
 
 ; Mix parameter that's a vector of integers and pointers of the same size
 declare void @test13a(<2 x i64>)
 
 define void @test13(<2 x i32*> %A) {
 ; CHECK-LABEL: @test13(
-; CHECK: call void bitcast
+; CHECK: call void @test13a
   call void bitcast (void (<2 x i64>)* @test13a to void (<2 x i32*>)*)(<2 x i32*> %A)
   ret void
 }
 
 ; Mix parameter that's a vector of integers and pointers of the same
 ; size, but the other way around
 declare void @test14a(<2 x i8*>)
 
 define void @test14(<2 x i64> %A) {
 ; CHECK-LABEL: @test14(
-; CHECK: call void bitcast
+; CHECK: call void @test14a
   call void bitcast (void (<2 x i8*>)* @test14a to void (<2 x i64>)*)(<2 x i64> %A)
   ret void
 }
 
 
 ; Return type that's a vector
 define <2 x i16> @test15a() {
   ret <2 x i16> zeroinitializer

llvm/test/Transforms/InstCombine/load.ll

;
   ret i64 %X
 }
 
 declare void @use.v2.p0(<2 x i8*>)
 declare void @use.v2.p1(<2 x i8 addrspace(1)*>)
 
 define <2 x i64> @test23(<2 x i64>* %P) {
 ; CHECK-LABEL: @test23(
-; CHECK-NEXT:    [[P_PTR:%.*]] = bitcast <2 x i64>* [[P:%.*]] to <2 x i8*>*
-; CHECK-NEXT:    [[X:%.*]] = load <2 x i64>, <2 x i64>* [[P]], align 16
-; CHECK-NEXT:    [[Y:%.*]] = load <2 x i8*>, <2 x i8*>* [[P_PTR]], align 16
-; CHECK-NEXT:    call void @use.v2.p0(<2 x i8*> [[Y]])
+; 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]])
 ; CHECK-NEXT:    ret <2 x i64> [[X]]
 ;
   %P.ptr = bitcast <2 x i64>* %P to <2 x i8*>*
   %X = load <2 x i64>, <2 x i64>* %P
   %Y = load <2 x i8*>, <2 x i8*>* %P.ptr
   call void @use.v2.p0(<2 x i8*> %Y)
   ret <2 x i64> %X
 }