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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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(""));
 
----------------
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.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55147