[PATCH] D107148: [InstCombine] Fold two-value clamp patterns

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D107148#2975017 <https://reviews.llvm.org/D107148#2975017>, @qiucf wrote:

> In D107148#2964910 <https://reviews.llvm.org/D107148#2964910>, @spatel wrote:
>
>> Sorry for missing this review earlier.
>> I implemented the corresponding folds for intrinsics with:
>> https://reviews.llvm.org/rG025bb5290379
>>
>> Can you confirm that the pattern matching and tests here correspond to those (replace the intrinsics with cmp+sel or vice-versa)?
>
> Yes, it does similar thing to this. So will it be better to fold such cmp-select pattern to the intrinsic?

We intend to canonicalize cmp-select to min/max intrinsics, but we need to address some more regressions that are visible in:
D98152 <https://reviews.llvm.org/D98152>

So it's a question of timing/duplication - when we get D98152 <https://reviews.llvm.org/D98152> done, then this patch probably becomes obsolete because we won't see the cmp-select pattern any more.

Ah - I just realized the tests here are pre-committed, so we can see what effect converting to intrinsics will have on them in D98152 <https://reviews.llvm.org/D98152>. 
It looks like we get some of the examples, but miss some others. So we will need to improve the analysis of the intrinsics for 2-way clamps either way. If you want to investigate that, that would be great.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3100
+    if (auto *C1 = dyn_cast<ConstantInt>(TrueSI->getFalseValue()))
+      if (auto *C2 = dyn_cast<ConstantInt>(FalseVal)) {
+        Value *Inner, *N, *InnerC;
----------------
qiucf wrote:
> RKSimon wrote:
> > It'd be better if we could use m_APInt here to support vectors (including handling undef element cases).
> Hmm.. Does `match(xxx, m_APInt(&x))` support vector type?
Partly - m_APInt will automatically match a splat constant <42, 42, 42,...>, but not an arbitrary vector constant.
For this fold, matching a splat is probably sufficient. 

m_APInt is basically free in terms of coding compared to m_ConstantInt, so we prefer to use m_APInt over m_ConstantInt for more general matching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107148