[PATCH] D68328: Fix occurrences that size and range of pointers are assumed to be the same.

[Joseph Faulls via Phabricator via llvm-commits]

Joe marked an inline comment as done.
Joe added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-custom-dl.ll:11
 ; CHECK-NEXT:    [[GEP1:%.*]] = getelementptr inbounds i8, i8* [[FOO:%.*]], i32 8
-; CHECK-NEXT:    [[TMP1:%.*]] = ptrtoint i8* [[GEP1]] to i32
-; CHECK-NEXT:    [[USE:%.*]] = zext i32 [[TMP1]] to i64
+; CHECK-NEXT:    [[TMP1:%.*]] = ptrtoint i8* [[GEP1]] to i40
+; CHECK-NEXT:    [[USE:%.*]] = zext i40 [[TMP1]] to i64
----------------
theraven wrote:
> delena wrote:
> > Joe wrote:
> > > delena wrote:
> > > > This change does not work for us. 
> > > > Doing this change we added a way to specify type for "ptrtoint" transformation and for size of GEP index. If you do not specify the index type, this change should be NFC. What exactly does not work for you?
> > > I don't believe that size of GEP index and type for "ptrtoint" transformation are the same thing. At least, that's how I interpreted langref: // 'The ‘ptrtoint’ instruction converts value to integer type ty2 by interpreting the pointer value as an integer'//. 
> > > 
> > > InstCombine was transforming 'ptrtoint i8* to i64' to 'ptrtoint i8* to i32', followed by a zext. Here, where the pointer size is 40 bits, that would mean 8 bits have been chopped off.
> > > 
> > > If you do not specify the index type, this change is NFC because index size defaults to pointer size.
> > I agree that we loose 8 bits in this case. Our promblem is in  i40 which is not an integer and we do not have any arithmetic ISA for i40.
> > Your current changes may cause many problems in our (out-of-tree) compiler.
> > Now we all have about 3 weeks local hollidays and we'd like to check your changes interlally when we back. So we whould ask you to hold on with this fix meanwhile.
> I am not entirely sure what the transformation that's happening in this test is supposed to show (I would be very happy if we required explanatory comments in tests!), but if the pointer range and the pointer size are not the same then I think any transformations involving `ptrtoint` / `inttoptr` are unsound.  The only information that this gives us is that a pointer is not just an integer address.  It may be an integer address that is sign / zero extended, an integer address that has some metadata in the high bits, a fat pointer, and so on.  
I think this test was originally written to check that Instcombine is behaving correctly when specifying pointer index range. I.e (in)correctly transforming a ptrtoint instruction to an integer size of that of the pointer index size. (Though, with this change, it would be the pointer size instead)

> if the pointer range and the pointer size are not the same then I think any transformations involving ptrtoint / inttoptr are unsound
I'm sorry, I don't quite understand this. Why would they be unsound? Pointer index range surely should have nothing to do with converting pointer values to/from integers.







Repository:
  rL LLVM

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

https://reviews.llvm.org/D68328