[PATCH] D147078: [LICM][BPF] Disable hoistMinMax() optimization for BPF target

[John Fastabend via Phabricator via llvm-commits]

jrfastab added a comment.

In D147078#4241741 <https://reviews.llvm.org/D147078#4241741>, @ast wrote:

> In D147078#4240301 <https://reviews.llvm.org/D147078#4240301>, @nikic wrote:
>
>> In D147078#4240289 <https://reviews.llvm.org/D147078#4240289>, @fhahn wrote:
>>
>>> I think adding a flag doesn't address the concern that were raised about making this behaviour target-dependent in general. I don't feel super strongly about the issue, so if other people in general think we should add a way to disable this transform, this is fine with me.
>>>
>>> But I think it would be better to undo the transform if needed. There is no guarantee that LICM is the only source of this pattern and it's generally considered that backends should support any form of valid LLVM IR.
>>
>> Agreed. In some ways this is even worse than the TTI hook because now you're modifying a cl::opt option from TTI, which is not how cl::opt is supposed to be used. This means that for example if you use BPF and some other target in the same process, this option is going to leak across them, not to mention that the modification is probably not thread-safe.
>>
>> The only proper way to address this is via an undo transform.

[...]

> Backends absolutely need knobs to control profitability of transformations. Undoing optimization in the backend is not practical. In many cases it's not possible to undo.

I think the idea here is that not all transformations are generically good for all backends. There is a cost model for instructions could there be something similar for transformations as well? Then BPF could mark the cost here somewhat high noting it doesn't actually provide value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147078