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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp abandoned this revision.
stefanp added a comment.

Thank you both for looking at this patch.
Looking at the comments I feel like I made an incorrect assumption about NULL pointers in this case and I'm just going to abandon this patch.

However, I have also replied to the comments below.



================
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())
----------------
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?



================
Comment at: llvm/test/Transforms/LoopStrengthReduce/Power/opaque-null-ptr.ll:2
+; RUN: llc -O3 -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 \
+; RUN:   -stop-after=loop-reduce %s -o - | FileCheck %s
+
----------------
nikic wrote:
> As this is just testing the IR transform, it would be better to use `opt -S -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 -loop-reduce < %s` or so (in which case you can also use update_test_checks.py).
I did try this initially but for some reason I was getting different results for this test. 

Anyway, I plan on abandoning this patch so I'm not going to look further into exactly why I am getting different results.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/Power/opaque-null-ptr.ll:27
+  %uglygep = getelementptr i8, ptr null, i64 %i184
+  %uglygep2 = getelementptr i8, ptr %uglygep, i64 4
+  %i190 = load float, ptr %uglygep2, align 4
----------------
efriedma wrote:
> Does this really reflect actual user code?  The first iteration of the loop loads from the constant addresses "4" and "8".
This is actually a reduced test from user code.
Unfortunately the resulting test is not realistic user code because the way that it was reduced discarded a lot of the code that would have given the function any real meaning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133426