[PATCH] D86262: [LoopIdiomRecognizePass] Options to disable part or the entire Loop Idiom Recognize Pass

Mon Aug 24 09:07:37 PDT 2020

fhahn added a comment.

Thanks for sharing the examples, IIUC they illustrate the issue with the cost modeling, which is the main motivation for the patch, rather than DA issues mentioned initially?

In D86262#2233615 <https://reviews.llvm.org/D86262#2233615>, @etiotto wrote:

> Transforming a loop into a memset or memcpy is not always profitable (it depends on how many elements are initialized/copied and on the efficiency of the target architecture implementation for those libraries). The low level optimizer should change short memset/memcpy back into a sequence of assignments, IMO this should not be done in opt because the exact length for which memset is less profitable  than individual assignment is a function of the target architecture. As for this PR, adding an option to disable the optimization is a good thing as it provides more flexibility to users that for whatever reason do not want the transformation to run (and is also handy for compiler developers when debugging code). And more flexibility is a good thing.

The option to disable the pass seems indeed convenient in the short term, but I am not sure if it is really helpful in the long run. It does not address the underlying issue (bad/non-existing cost model) and it means we generates sub-optimal code  in some cases for the vast majority of users (which will neither know nor set this special flag). Also, will the option work with LTO? I think Clang usually does not pass `-mllvm` options to the linker/LTO plugin.

I think such an option is fine as a temporary stop-gap solution, but the goal should be to fix the underlying issue IMO. Otherwise I am worried that the option reduces the incentive to fix the cost-modeling (which should be a fix-able issue).

It looks like there are some un-addressed comments with respect to the exact way to disable the transformation. If there's a single option to disable the whole pass, it might be worth considering to not add it to the pipeline, rather than bailing out in the pass. Also, it would be good to update the description of the patch with the actual motivation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86262