[PATCH] D154051: [LangRef] Always allow getelementptr inbounds with zero offset

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D154051#4463060 <https://reviews.llvm.org/D154051#4463060>, @nikic wrote:

> In D154051#4463038 <https://reviews.llvm.org/D154051#4463038>, @nlopes wrote:
>
>>   +  LLVM :: Transforms/InstCombine/gep-inbounds-null.ll
>>   define i1 @test_eq(ptr %base, i64 %idx) {
>>     %gep = gep inbounds ptr %base, 1 x i64 %idx
>>     %cnd = icmp eq ptr %gep, null
>>     ret i1 %cnd
>>   }
>>   =>
>>   define i1 @test_eq(ptr %base, i64 %idx) {
>>     %cnd = icmp eq ptr %base, null                ; wrong since %base can be OOB
>>     ret i1 %cnd
>>   }
>
> Hm, I don't think I understand why that transform is not valid anymore. Going through the different cases here:
>
> - `%base == null`, `%idx == 0` -> `%cnd == true`.
> - `%base == null`, `%idx != 0` -> `%cnd == poison`.
> - `%base != null`, `%idx == 0` -> `%cnd == false` (by assumption).
> - `%base != null`, `%idx != 0` -> `%cnd == false`. This is because it can only be true for `%idx = -ptrtoint(%base)`, which cannot be inbounds in the default address space, as no allocated object can start at address zero.
>
> So I think reducing this to `%base == null` remains correct?

You're right. I overlooked this. Alive2 was doing an internal optimization that is no longer valid with this semantics.
So we're down to 1 easy regression then.

I'll run Alive2 over a few larger programs just in case, but I think we can move on. I'll report back if it finds more regressions.


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

https://reviews.llvm.org/D154051