[PATCH] D128582: Enable SeparateConstOffsetFromGEPPass() at -O3 and -O2

[Shubham Narlawar via Phabricator via llvm-commits]

gsocshubham added a comment.

In D128582#3624787 <https://reviews.llvm.org/D128582#3624787>, @dmgreen wrote:

> Hello. Unfortunately, I doubt that people will be in favour of this approach, especially if it is introducing ptr2int and int2ptr's so early in the pass pipeline. It looks like a pass that needs to be run as part of the backend.
>
> There is a run of the pass in the AArch64 backend already, but it is disabled by default: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp#L581
> Enabling it didn't seem to help with the original case in #50528. Would it be possible to teach it what it needs to for that case, and from there enable the EnableGEPOpt flag?

Regarding teaching `SeparateConstOffsetFromGEPPass` pass to handle constant GEP's -

>From #50528, there is only one case where there is gep with all constant indices as below -

1. `store i32 %inc, i32* getelementptr inbounds (%struct.state_t, %struct.state_t* @s, i64 0, i32 2)`

-> At first, this was not being considered for gep as it is store instruction and hence I split above into 2 instructions in testcase as below -

  %temp = getelementptr inbounds %struct.state_t, %struct.state_t* @s, i64 0, i32 2
    store i32 %inc, i32* %temp



2. `%temp` is not being considered by the pass since it has all constant indices -

  // The backend can already nicely handle the case where all indices are
  // constant.
  if (GEP->hasAllConstantIndices())
    return false;

I tried forcefully splitting it by passing the checks but it does not seem right to split GEP with all constant indices at IR level. From the comment backend passes should take care of it but I still see repeated instructions -

  madd    x8, x8, x10, x9
  madd    x0, x11, x12, x8

I see 2 occurrencea of above set of instruction in assembly. Ideally it should occur only once.

Reference - https://clang.godbolt.org/z/KfrfT97hn


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

https://reviews.llvm.org/D128582