<table border="1" cellspacing="0" cellpadding="8">
    <tr>
        <th>Issue</th>
        <td>
            <a href=https://github.com/llvm/llvm-project/issues/54554>54554</a>
        </td>
    </tr>

    <tr>
        <th>Summary</th>
        <td>
            InstCombine and CodeGen do not respect -fno-builtin-fmax
        </td>
    </tr>

    <tr>
      <th>Labels</th>
      <td>
            new issue
      </td>
    </tr>

    <tr>
      <th>Assignees</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Reporter</th>
      <td>
          irichter
      </td>
    </tr>
</table>

<pre>
    The InstCombine optimization [detects](https://github.com/llvm/llvm-project/blob/ccb54d5b42031dc05fc2dd224be70399cc512245/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L2857) cases where there is an attempt to use a conditional operator to return a minimum or maximum floating point value (e.g.: `(x > y) ? x : y`), and replaces it with the appropriate `llvm.maxnum` intrinsic. However, CodeGen can only [convert this intrinsic into a libm libcall](https://github.com/llvm/llvm-project/blob/ccb54d5b42031dc05fc2dd224be70399cc512245/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp#L826), without respect to any `no-builtin-*` function attributes.

This makes compiling a [`fmax` function from libm](https://github.com/riscv-collab/riscv-newlib/blob/83d4bf04bb35e963d3412355a09810760b5851ce/newlib/libm/common/s_fmax.c) with `-O2` somewhat futile, as the compiled result simply tail-calls itself.

I have a somewhat kludgy patch, which prevents the above issue, but does not properly distinguish between `fmax`, `fmaxf`, and `fmaxl`. Some more thought should probably also go into whether or not to allow the transformation if for some reason the function associated with the instruction cannot be found. (The code below allows it, and only conservatively prohibits the conversion if it's sure that there is a relevant `no-builtin` attribute.) Maybe the better patch would instead modify CodeGen to generate a sequence equivalent to `fmax` if it should be used.

```
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 5bbc3c87ca4f..26e69196cc83 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3058,14 +3058,20 @@ Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
   // minnum/maxnum intrinsics.
   if (isa<FPMathOperator>(SI) && SI.hasNoNaNs() && SI.hasNoSignedZeros()) {
     Value *X, *Y;
+    BasicBlock *BB = SI.getParent();
+    Function *F = (BB)?BB->getParent():nullptr;
+    if (!F || !F->hasFnAttribute("no-builtin")) {
       if (match(&SI, m_OrdFMax(m_Value(X), m_Value(Y))))
+        if(!F || !(F->hasFnAttribute("no-builtin-fmax") || F->hasFnAttribute("no-builtin-fmaxf") || F->hasFnAttribute("no-builtin-fmaxl")))
           return replaceInstUsesWith(
               SI, Builder.CreateBinaryIntrinsic(Intrinsic::maxnum, X, Y, &SI));
 
       if (match(&SI, m_OrdFMin(m_Value(X), m_Value(Y))))
+        if(!F || !(F->hasFnAttribute("no-builtin-fmin") || F->hasFnAttribute("no-builtin-fminf") || F->hasFnAttribute("no-builtin-fmaxl")))
           return replaceInstUsesWith(
               SI, Builder.CreateBinaryIntrinsic(Intrinsic::minnum, X, Y, &SI));
     }
+  }
 
   // See if we can fold the select into a phi node if the condition is a select.
   if (auto *PN = dyn_cast<PHINode>(SI.getCondition()))
```
</pre>
<img width="1px" height="1px" alt="" src="http://email.email.llvm.org/o/eJzVV1tv2zYU_jXKC2FD1sWXBz_YztwaWNMAybZ2LwElURZXStREyq776_cdSrKdtEDbdUAxQ7B0RPJcvnNVorPT8rEQbFcZu9FlIivBdG1lKT9xK3XFvHidCStSa7z41gvmhbW18cKVF2xx7aUt2mSc6hKEUofhNqob_RcOgUyUTnBL0ySOsjiJAj-cZKkf52mQZUEQJWLmh4tFmsYTUPEVH0nnHhtemVw3pQFxpeVz6kEoSBunde0F4a_BPJ55wYKl3AjDjoVoBLPuXxrGK8atFWVtmdWsNYJxluoqk2QuV7BeNNzqhlYbYdsG-1kpK1m2JcPrkn90j7nSQKjas1rLyrIDV61gAEiM92Pgw7ypD-oj88Jf2Im08cItI3IFktYWXrCBNhmk1Iqn0FRadgSgpCvjNSCsG8mtIFaEyRiiq7YExSCxkZWR6Zi91kdxEA0x2-hMvBIV7K6YrtSJnAfTsApbC9h-PkZPGnYB45L-Uq7Uz_FvrzOeXoERVzu4EsSvYs-V_CSa10LBIYNn58G0B46A0q0FeKaGIuQtXp0IqkqPklYq-GbkBStCK2-r1AUzHN_IpLXCjD3_1vNX3f8jYVPyD3ABLK2lIrdyQg-nc6D-jEneaIdZ-VXAGmnSwyjVSvHkTFbi2FneAzcPsyjJ_ShJwlgspmEWRpMgjGPuL-YTfzb1k3geT1IK-PNRJx2Y67LUBJ15Ii3HKYWZiyAoPHobkNpGl-JYcAv9rVTCxZxxEdaZKij-TKssM7KsETOWSzWieKB4NELlz6DasYIfKGXOfD-oNtufWM1tWjjHFDItWN0gKCvbSeKJPlDumdbJhwNYpgF2pS2jKBcN5GbSUDq10hQsEfYoEMgX-OlcT-U9SanTv1K4j9kDVGKldsmu230Bk3BXGclIeAIZXBnN9rqLftQFKgqU06QIBZBS-ug0tkPV6YqgzBkIZzPQ4gavaNclrozRKaVqdklg5Jlt2m4dCUkiEhzRbZWNqU48OhdkAm9JqpNNkA-muQRG9hrRHKDFQYCEIYVMpB0cSKltegXp5Mww0zr7ub2qeNBZiQNHlXqWHhQe54wYU-y84afElUrygAU2zqvs6FAkgwTPgHAm89O52AC3vaioZrqwEH-3okoFw12iJiIGaMdVHjlVB89AGipw9izEyL3d5UgIy9lohMRi_EebA0t-uL04nWSViY8sTpI0TOezlEf5eBxMxXQxWUzTdB6yie9Po6jbPBqNflzzHppg3V3_lSFe5ONio9CP5wi8SYTIXPdE4LN-eXcVyiipV_yaHWoGFb9wdZBG2o4_bUCEXwicmj7sXBOcrTvJjLGuYlJvpa4WbLv2dmlSQ5HGVgQNGErDvXCzvX_DbfG2b9NorySqY07dYcoeduOCmzt9x-8AxvwLKw9yX4nsT9HofsNL1Rj7ve_nq3eu9ASr9164PnuBdqw5dFwrnX6g5fUarf2WJOyFvecNAr9n_eLYdigaOLR1Z7BvvXY7t-v1CAZ9xmFVtUrVtnnJq4PFCyZgNNvgYvRMLGDmtloNye02BVeZHwRfNHrgWHbFfN67bcPKp7dNtn2DDMbik8MGT-_6Xnx5875j21_XqnbMP9cWb75B4ZGrHk7r4ey3nsr_3TF1xuhsCbv8-sGwn9woxH_DqPkHaj8xe7mbfh2Oa0jIMM5s0EWsWMuKN6fdEO84enl2OdUPfDjoovB9F4pdKl2HFvsOH5L3f5YPh8j7PmfI6n_tw768fdWH9PNmt1eInyn2smY-CEFuPgo36-carZS6tnEVdxju60Jissnczn5e6L5yuqGg2_yyxvKW2nWwur9ztSk7VU_4jLKou_evd3fgNhRcqnObgeO5il5CZujiN9kyzBbhgt9YaZVYXn9p0pwzzBGZdmPYMM-P8hfOvGkbtfzurxM3dFJLjKM4jm6KpR9F-SyfRCL302mYJrN4MYmiyA_RxqcZD24wqwtlljT8I6DEcZhbA4z7N3IZ-AG-cIJ4EsbhZD72J4t8OuHhJJhPp0lG7VKUmKDH7oNNN_ubZulUStq9waLClGsuixgcqRUJJw78gX6hm6VsMENj_rpxspdO938ABdPeyA">