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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1913-1914
   EVT VT = Sel.getValueType();
+  if (SelOpNo && VT != C1.getValueType())
+    return SDValue();
+
----------------
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.


https://reviews.llvm.org/D48223