[PATCH] D9107: Fix CombineToPostIndexedLoadStore in DAGCombiner.cpp

[Quentin Colombet]

Hi François,

Either way sounds good to me, depends what is most comfortable for you I guess.

See my comments inlined for few hints/remarks.

> On Jul 9, 2015, at 12:33 AM, Francois de Ferriere <francois.de-ferriere at st.com> wrote:
> 
> fdeferriere added a comment.
> 
> Hi Quentin,
> 
> Thanks for your detailed review.
> 
> I agree with you that this patch introduces too much regressions to be delivered. I see now two possibilities :
> 
>  1- Still rely on this pass to select pre/post addressing modes, but add a kind of "repair" pass after code selection to fix the regressions. This is what we did on our target. However, I am not sure if on ARM this could fix the code to catch again the multiple load/store instructions,

This is what is done on ARM as well with the load store optimizer. You may be able to patch up this pass to recover the regressions. That being said, that may be a difficult task as the load store optimizer does not handle the auto mod variant IIRC.

> 
>  2- Disable this pass and add a new pass on machine instructions. The main advantage I see is that it can allow to look for pre/post patterns on regions larger than a DAG, which currently is a limitation. For our target, it would probably replace our "repair" pass.

I wonder if the machine combiner would help here. You may want to give it a try.

> 
>   Should I add a comment "Abandon Revision" to close this patch ?

If you think you can patch up the load store optimizer, I would probably keep that revision around so that you can land it when the regressions are fixed. Otherwise, yes, close this with Abandon Revision.

>   Should I log a kind of "request for enhancement" (in Bugzilla ?) to keep a trace of this analysis ?

Good idea!

Cheers,
-Quentin

> 
>  Regards,
> 
>     - François.
> 
> 
> http://reviews.llvm.org/D9107
> 
> 
>