[PATCH] D67799: [InstCombine] Fold a shifty implementation of clamp0.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1933-1936
+    if (match(&I, m_c_And(m_Value(Shift), m_Value(V))) &&
+        match(Shift,
+              m_OneUse(m_AShr(m_OneUse(m_NSWSub(m_Zero(), m_Specific(V))),
+                              m_APInt(ShAmt)))) &&
----------------
lebedev.ri wrote:
> 1. Only `m_AShr` has to be one-use
> 2. This doesn't actually deal with commutativity correctly
> 
> You want
> ```
> match(&I, m_c_And(m_OneUse(m_AShr(m_NSWSub(m_Zero(),
>                                            m_Specific(V)),
>                                   m_APInt(ShAmt))),
>                   m_Value(V)))
> ```
Err, 
```
match(&I, m_c_And(m_OneUse(m_AShr(m_NSWSub(m_Zero(),
                                           m_Specific(V)),
                                  m_APInt(ShAmt))),
                  m_Deferred(V)))
```
of course


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1940
+          Builder.CreateICmpSLT(V, ConstantInt::getNullValue(Ty));
+      return SelectInst::Create(NewICmpInst, ConstantInt::getNullValue(Ty), V);
+    }
----------------
lebedev.ri wrote:
> Hmm, super random thought.
> @spatel we convert code that was written without a branch, likely very intentionally,
> into a possibly-branch code. Should we not add `unpredictable` to this new `switch`?
> I think it's almost correctness question..
s/switch/select/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67799