[PATCH] D55147: Exclude non-integral pointers in isBytewiseValue

Cherry Zhang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 4 14:27:45 PST 2018


cherry added inline comments.


================
Comment at: include/llvm/Analysis/ValueTracking.h:227
   /// return undef.
-  Value *isBytewiseValue(Value *V);
+  Value *isBytewiseValue(Value *V, const DataLayout &DL = DataLayout(""));
 
----------------
efriedma wrote:
> cherry wrote:
> > Provided a default value as this function is public so it won't break external uses. External uses probably don't care about non-integral pointer anyway. (E.g. clang has one use.)
> > 
> I'd prefer not to do this; constructing a DataLayout like this is probably sort of expensive.  It's not a big deal to update clang.
Ok, will do.


================
Comment at: test/Transforms/MemCpyOpt/non-integral-ptr.ll:19
+  %2 = bitcast i8 addrspace(1)** @const0 to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %1, i8* align 8 %2, i64 8, i1 false)
+  ret void
----------------
efriedma wrote:
> cherry wrote:
> > efriedma wrote:
> > > It's not clear to me why a memset is "wrong" here; either way, you're setting all the bits to zero.  Is the issue that const0 is not actually an all-zero constant?
> > > 
> > > The LLVM null pointer constant is treated as an all-zero value in LLVM, even on targets where a null pointer constant in C would have some non-zero value.  If you think that isn't appropriate, please start a thread on llvmdev; I haven't really thought it through completely, but it would probably get complicated due to the impact on various transforms like constant folding.  If you need an "opaque" null pointer constant, there's probably a simpler solution: define a special global @llvm.opaque.nullptr or something like that, and make your frontend use it instead of ConstantPointerNull.
> > Thanks for the comment.
> > 
> > Generating memset isn't immediately wrong, but other parts of the optimizer, like store-to-load forwarding, may try to do further optimizations and bad things could happen. For example, it may construct an integer 0 and convert to non-integral pointer type, which fails, or do other things that invalidate the non-integral pointer semantics.
> > 
> > I'm not suggesting that it should have a non-zero value. It is a zero value, just cannot be converted to/from integer zero value.
> > 
> > Also, there is a similar test test/Transforms/LoopIdiom/non-integral-pointers.ll that checks memset is not used for non-integral pointers.
> > 
> Either the bits of a null pointer can't be inspected at all, or optimizations can assume the bits are actually zero; there is no in-between state.  And whichever choice is correct, changing isBytewiseValue like this doesn't help make LLVM consistent.  If the bits can't be inspected, the IR shouldn't contain ConstantPointerNull, and isBytewiseValue won't be able to prove anything anyway.  If the bits can be inspected, isBytewiseValue is already returning the correct answer.
> 
> That said, this doesn't really have any impact on targets which don't have non-integral pointers, so I don't really care that much about what happens here; maybe this is helpful in practice even if it isn't logically consistent.
I'm not sure why ConstantPointerNull implies the bits can be inspected, or why memset must be used.

In the documentation it says non-integral pointers "have an unspecified bitwise representation". If the zero value of non-integral pointer, unlike all other values, has a defined bitwise representation, it probably need to be documented this way, although it looks weird to me. Maybe @reames could help clarify the semantics.

Thanks.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D55147





More information about the llvm-commits mailing list