[PATCH] D127726: [InstCombiner] Add option to replace PHI of GEPs with GEP with PHI as index

Thu Jul 14 13:39:21 PDT 2022

nikic added a comment.

In D127726#3653058 <https://reviews.llvm.org/D127726#3653058>, @spatel wrote:

> In D127726#3651809 <https://reviews.llvm.org/D127726#3651809>, @eklepilkina wrote:
>
>>> We already have an implementation of this general transform -- shouldn't it be sufficient to relax only this condition? https://github.com/llvm/llvm-project/blob/7dc18a62e40e241019ec77e70f01bc41d39ab748/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp#L537 (At least for the basic case.)
>>
>> Yes, I saw this code and make several experiments with it. But decided to add extra method. Let me try to describe my motivation.
>>
>> 1. Orginal fold methods use the approach that they check the first PHI operand. And work with cases when there are only GEPSs in PHI node. We need the case when there can be both GEP instructions and just array.
>> 2. My experiments show that really in many coditions such modification makes worse (as mentioned in comment for condition you have provided a link for). If you looked at code we have added several conditions that are connected with uses of his PHI node and number of operands. As far I had to add all these not very obvious heuristics in this modification I decided to do it as separate method. Moreover, then it is easier to turn on/off it with flag. If integrate flag  and all needed heuristics inside exsiting method, it becomes not so readable.
>
> I haven't looked at the patch in detail, but the comments suggest this is not moving in the right direction. There should not be a debug flag to enable this transform unless there are planned follow-ups that would allow removing the flag soon after the initial commit. If the transform needs heuristics to be profitable, then it's probably not a good match for instcombine (canonicalization). The backend or later passes would have to be able to invert the transform to avoid regressions.

I believe we should be able to do this as a canonicalization (at least dropping the constant bailout for the existing transform -- not sure about the case where not all inputs are GEPs) and undo in the backend -- this is the usual problem that in IR we want to form constant phis, while the backend prefers pushing operations into phis, because it often avoids constant materialization "for free". There have been multiple attempts at that, the latest being D119916 <https://reviews.llvm.org/D119916>. I still think the right approach to this problem is a late backend IR pass.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127726