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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D147078#4231270 <https://reviews.llvm.org/D147078#4231270>, @anakryiko wrote:

>> If so, I think it would make the most sense to add an undo transform in the BPF backend.
>
> Genuine curiosity (and keep in mind I'm no compiler expert at all), but why doing transformation and then customly undoing it in specific BPF backend would be a preferable solution to allowing backends to turn off some known-to-hurt transformations in the first place? It seems more complicated and error-prone to do the dance of undoing, rather than have a generic interface to prevent transformation in the first place.
>
> Note, this is not the only transformation that hurts BPF verification, @yonghong-song previously added "undo transformation" logic, but it seems like a not very sustainable and very convoluted solution. So we are asking if we can have a simpler and more reliable opt-out for some small set of transformations.

Just blocking a transform is generally insufficient, because it does not handle the case where the code uses the problematic pattern in the first place. If the input code already contained the `x < min(y, z)` pattern, it would fail the verifier and converting it into `x < y && y < z` would be necessary to make it pass.

If certain canonicalization transforms are undesirable for a specific target, this is always addressed by adding backend undo transforms, not by disabling canonicalizations. Use of TTI hooks inside canonicalization passes is generally rejected as a matter policy -- you need a very good case for that (e.g. an undo transform being impossible for some reason).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147078