[PATCH] D153963: [InstCombine] Fold binop of select and cast of select condition

[Antonio Frighetto via Phabricator via llvm-commits]

antoniofrighetto added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:913
+
+  if ((match(RHS, m_UIToFP(m_Value(A))) || match(RHS, m_SIToFP(m_Value(A)))) &&
+      A->getType()->getScalarSizeInBits() == 1) {
----------------
nikic wrote:
> I don't think we should support uitofp/sitofp in this fold -- this seems way too contrived. It would be okay if this fell out of some generic handling, but not something we should spend lines of code on.
@nikic, I do not have any strong opinions here, no problem removing them, I just thought it could make sense to support fp ops, as follows:
```
float fadd_select_uitofp(unsigned int a, bool b) {
    float f1 = b ? 2.0 : 1.0;
    float f2 = (float) b;
    return f1 + f2;
}
```
Considering that the patch is meant to support the following use case – amongst the others:
```
int add_select_zext(unsigned int a, bool b) {
    int f1 = b ? 2 : 1;
    int f2 = (int) b;
    return f1 + f2;
}
```
They seem like less than 30 LOCs to me, they should not really harm. Might be a bit contrived, but it is still valid C code.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:943
+  return nullptr;
+}
+
----------------
goldstein.w.n wrote:
> This entire function is in desperate need of comments.
> Likewise the summary (commit message) should actually explain the transform being added.
> 
> Please comment this at the very least to make review simpler :)
Thanks for the note, apologize for that, I'm fixing it right away to introduce more comments.


================
Comment at: llvm/test/Transforms/InstCombine/binop-select-cast-of-select-cond.ll:197
+  ret <2 x i64> %add
+}
----------------
goldstein.w.n wrote:
> Can you split the tests to a seperate commit so we can see the diff generated by this patch?
Do you mean to open a different patch for the tests (or land them directly)?


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

https://reviews.llvm.org/D153963