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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D154051#4463038 <https://reviews.llvm.org/D154051#4463038>, @nlopes wrote:

> I've implemented this semantics in Alive2 and run it over LLVM's test suite.

Thanks for testing!

> There are only 2 regressions, and they seem ok:
>
>   +  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?

>   +  LLVM :: Transforms/InstSimplify/gep.ll
>   define ptr @undef_inbounds_var_idx(i64 %idx) {
>     %el = gep inbounds ptr undef, 8 x i64 %idx
>     ret ptr %el
>   }
>   =>
>   define ptr @undef_inbounds_var_idx(i64 %idx) {
>     ret ptr poison                                 ; only ok if %idx != 0
>   }

Agree that this is no longer correct, we need to return `undef` instead. That's okay as we can still return `poison` for a `poison` base.


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

https://reviews.llvm.org/D154051