[PATCH] D49242: [Intrinsics] define funnel shift IR intrinsics + DAG builder support

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5695
+    SDValue ShY = DAG.getNode(ISD::SRL, sdl, VT, Y, IsFSHL ? InvShAmt : ShAmt);
+    setValue(&I, DAG.getNode(ISD::OR, sdl, VT, ShX, ShY));
+    return nullptr;
----------------
fabiang wrote:
> Wait, I don't think this lowering works when Z=0 (mod BitWidth) when BitWidth is the width of the register; not on shift-amounts-modulo-register-width architectures anyway. (It should work on PPC.)
> 
> e.g. for 32-bit fshl and with shift widths taken modulo 32, we get (X << 0) | (Y >> 32) == (X >> 0) | (Y >> 0) == X | Y, not the expected X.
> 
> For compile-time constant Z the Z=0 case can be handled, and when X=Y (i.e. rotate) it turns out to be harmless, but for X!=Y and variable Z that happens to be congruent to 0 I think this is trouble.
Oops...I stepped in the UB this was supposed to avoid. And even if we fix it for rotates, we still have the problem for proper funnel shifts (X != Y) as you've noted. We'd have to select the 0-shift case out I think. 

Should we just go back to the original plan and make rotate intrinsics instead of funnel shift? I don't see much demand for the more general funnel shift.


https://reviews.llvm.org/D49242