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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

I'm fine with adding the option. However I think this patch is not solving the problem you have.

LICM is not the only pass that may potentially do it. For example, InstCombine may end up doing something similar. As people above have said, this is not a scalable approach. In general, any construction that is legal from LLVM's perspective may appear in your code. With this transform you are lucky: it's isolated and there is a clear place where to add a check. In general, it might not be the case.

I think you still need a patch that will undo the transform for this backend.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:995
       // expression out of the loop.
-      if (hoistMinMax(I, *CurLoop, *SafetyInfo, MSSAU)) {
+      if (EnableMinMaxHoisting && hoistMinMax(I, *CurLoop, *SafetyInfo, MSSAU)) {
         ++NumMinMaxHoisted;
----------------
Might as well in `hoistMinMax` do
```
  if (!EnableMinMaxHoisting)
    return false;
```
It's more reliable in case if someone else wants to use it in another place.


================
Comment at: llvm/test/CodeGen/BPF/licm-minmax-hoisted.ll:1
+; RUN: opt -O2 -mtriple=bpf-pc-linux -mcpu=v3 -S < %s | FileCheck %s
+; source:
----------------
Can you instead dump a dedicated test for LICM? For that, pass `print-before=licm print-module-scope`, and dump IR before LICM. You can also use bugpoint or llvm-reduce tool to make it smaller. It's not clear where exactly a problem in your test is supposed to happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147078