[PATCH] D76010: [ValueTracking] Let isGuaranteedNotToBeUndefOrPoison look into more constants/instructions

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4548
+  // Call stripPointerCastsSameRepresentation to take addrspacecast out of
+  // consideration.
+  auto *StrippedV = V->stripPointerCastsSameRepresentation();
----------------
aqjune wrote:
> jdoerfert wrote:
> > This is not 100% the idea behind `stripPointerCastsSameRepresentation`:
> > `stripPointerCastsSameRepresentation` says the bit-representation stays the same, not that there are no addrspacecasts stripped. If we assume an addressspacecast that is known not to modify the bit-representation will also not modify the poison/undef state, which I think is reasonable, we can do tihs.
> Yes, I think in that case undef/poison should be preserved. LangRef says that addrspacecast can be no-op, I believe this function is dealing with the case. 
> Updated comment
As I said, the function will strip (any) casts that are known not to change the bit representation. I agree that this should be fine here but the comment makes it sounds as if only some addrspacecasts are stripped.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4553
+    // gep inbounds null, 0 is also null.
     return true;
 
----------------
aqjune wrote:
> jdoerfert wrote:
> > misplaced comment
> Added more text to the comment, my underlying idea was that null was also allowed in this case as well.
I still find it misplaced (and I dislike comments in a single conditional stmt without braces). Where does the GEP come from and why is it inbounds?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76010