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

[Andrii Nakryiko via Phabricator via llvm-commits]

anakryiko added a comment.

>> 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.
>
> One goal of LICM is to target-independently canonicalise the IR to enable other IR optimizations, so IMO it would be preferable to avoid disabling parts via target hooks. Also, the currently proposed hook is not really scalable, e.g. what if another target wants to opt out as well?

I agree that we shouldn't do "isBPFTarget()" check like proposed in current version. See my comment below, where I proposed to have more generic "isTransformEnabled(enum transform_id)" method, which could be overriden by different backends. Then BPF would have a simple override in `BPFTTIImpl`:

  bool isTransformEnabled(transform_id id) {
    switch (transform_id) {
    case transform_id::HOIST_MIN_MAX:
      return false;
    default:
      return true;
    }

while most other backends will just have default "return true;" implementation.

Note that we don't have to enumerate all possible transforms in `enum transform_id`, only those that are optional.

Does it make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147078