[PATCH] D108988: [ARM] Simplify address calculation for NEON load/store

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

>> The way we handled this in MVE was to "distribute" the increments in the ARMLoadStoreOptimizer pass. The instructions in MVE are different, and that does involve checking through Machine Instructions for Adds that can be better distributed into postinc instructions. LSR got it mostly right, DAG Combine did an OKish job most of the time, and we fixed up what went wrong later in the pipeline.
>>
>> It seems to have worked out OK as far as I can tell, is there a reason we can't do the same thing here?
>
> I think the approach is still the same, the new pass just works for
> cases that LSR does not handle.

The MVE method works quite differently I feel. It fixes up problems later in the pipeline at the mir level, not trying to get them perfectly correct before ISel. It can be awkward dealing with mir though, and difficult to look through all the instructions that can be generated looking at ways to distribute postincs more evenly if the results are not already close enough. I don't think I would recommend it for this problem, it would probably be simpler to fix it up in the DAG than trying to do it later in MIR.

>> Adding the new pass seems fine if we need it, but I'm less sanguine about having to disable a lot of Add folds in DAGCombiner.
>
> Agree, this is potentially the most problematic change. It is limited
> to just (load/store (add)) and works only before legalization, so this
> /hopefully/ reduces its impact to just the patterns we need.

Unfortunately I'm not sure we can say this in general, even if it is quite rare. I feel like having this reassociationCanBreakPostIndexingPattern method in the generic DAG combine code disabling so many of the ADD folds is not a good sign. It feels like it's working around the fact that this is a quite fragile way of trying to get postinc working. The main issues I found I think could be fixed with one use checks in reassociationCanBreakPostIndexingPattern, but is there a way to make this work without disabling the generic folds? Perhaps by adding new folds for fixing postinc patterns, and getting "add-like-ors" to behave like adds in more places?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108988