[PATCH] D133426: [LSR] Address a special case involving NULL.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:3478
+      } else if (const Constant *Const = dyn_cast<Constant>(V)) {
+        // NULL values don't really have a live range. They are just NULL.
+        if (Const->isNullValue())
----------------
stefanp wrote:
> efriedma wrote:
> > NULL has a live range.
> > 
> > If you assume all the users of the NULL are GEPs, then I guess in practice the null is going to get eliminated... but GEPs from null should be extremely rare: it implies you're loading from an absolute address.  And it's undefined behavior to load from a null pointer unless the function is marked null_pointer_is_valid (or you're in a non-zero address space).
> Fair enough. I agree with you.
> This was actually my initial assumption and it seems to me that I was wrong. Therefore, I would like to abandon this patch. 
> 
> However, I would like to give more background on the motivation of the patch because that is what I was trying to address in the first place. I have been looking at switching some tests from typed pointers to opaque pointers. The issue I have found is that certain passes (including this one) behave differently and sometimes produce different code when we switch from typed to opaque pointers. The reason for this is that we are asking for the "users" of `ptr null` (here: `for (const Use &U : V->uses()) {`). 
> For example:
> ```
> %i1 = getelementptr inbounds i8, i8* null, i64 %adr1
> %f2 = getelementptr inbounds float, float* null, i64 %adr2
> ```
> vs. 
> ```
> %i1 = getelementptr inbounds i8, ptr null, i64 %adr1
> %f2 = getelementptr inbounds float, ptr null, i64 %adr2
> ```
> In the first example there is one use of `i8* null` and one use of `float* null`. However, in the second example we have two uses of `ptr null`. This then takes a different path through the compiler and we may end up with different code.
> 
> Anyway, I agree with you that GEPs from NULL (basically a form of `inttoptr`) are not common. However, should we be concerned about  this behaviour? Would we need to document it? Or it is just rare enough that we don't really care?
> 
If you're iterating over the use-list of "null", you're probably doing something which doesn't make sense, anyway... it should be rare enough that it doesn't really matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133426