[PATCH] D48223: Allow binop C1, (select cc, CF, CT) -> select folding

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1913-1914
   EVT VT = Sel.getValueType();
+  if (SelOpNo && VT != C1.getValueType())
+    return SDValue();
+
----------------
spatel wrote:
> rampitec wrote:
> > spatel wrote:
> > > When does this happen (is there a test)? Why does this only happen when the select is operand 1 of the binop? Better to remove the 'SelOpNo' condition to be safer?
> > That is if shift value and amount have different types. On x86 shift amount is i8 regardless of LHS. The new test shl_constant_sel_constants does not fold on x86 but does on amdgpu. W/o the check this test asserts. At the same time old test does work on x86 and folds correctly, so I think it is only needed if I have swapped operands. In general that should be possible to write a piece of code to create correct VT for shifts, but I'd better leave that to x86 folks. I guess range checking will be also needed if such a code is about to be written.
> Thanks - that was my guess, and the TODO + test makes it clear. 
> 
> LGTM, but get final approval from @arsenm once the AMDGPU tests are updated.
So @arsenm has reviewed the tests. @spatel it looks like your previous vote technically holds the review now. Can this be submitted?


https://reviews.llvm.org/D48223