[PATCH] D61607: Introduce an option to stripPointerCasts to force the same bit pattern

[Johannes Doerfert via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/IR/Value.h:504
+  const Value *stripPointerCasts(bool KeepBitPattern = false) const;
+  Value *stripPointerCasts(bool KeepBitPattern = false) {
     return const_cast<Value *>(
----------------
hfinkel wrote:
> jdoerfert wrote:
> > arsenm wrote:
> > > I think a separate function name would be more helpful, implemented with a hidden bool argument 
> > It depends on different things, including taste. I don't feel strong so I'd be fine with a new function as well. Two things to consider though:
> > 
> > Do we want to prevent as-cast stripping in other strip-casts scenarios as well?
> > This patch enables it for PSK_ZeroIndicesAndAliases but not for PSK_ZeroIndices and  PSK_ZeroIndicesAndAliasesAndInvariantGroups. If the answer is yes, then we would end up with even more versions in the stripXXXX family (we have a bunch already).
> > 
> > Do we want to be able to set `KeepBitPattern` as the default in order for us to "guarantee" correctness and give people a way to opt-in? We can achieve this also if the new function would look through as-casts and `stripPointerCasts` wouldn't anymore.
> > 
> > 
> > 
> If we want to be able to switch the default at some point, I certainly recommend something with a name so that we can find the various forms with grep (etc.). We can always add more forms as needed (or we can switch the argument type to an enum).
> If we want to be able to switch the default at some point, I certainly recommend something with a name so that we can find the various forms with grep (etc.). 

Later, the easiest "switch situation" is with the default argument (= this patch), especially because of down-stream code. Adding a new function now will fix the behavior of the new function. Switching `stripPointerCasts` later would then require us to replace __all__ uses or introduce yet another function as a way to look through as-casts.

> We can always add more forms as needed (or we can switch the argument type to an enum).

Now I'm confused. Do you want an argument to `stripPointerCasts` or a new function which will have a fixed semantic wrt. as-casts. For the latter, we would introduce either: `stripTrivialPointerCasts` (= w/o as-casts) or `stripAddressSpaceAndPointerCasts` (= w/ as-casts) and make `stripPointerCasts` do the oposite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61607