[PATCH] D110508: [SelectionDAG] Fix shift libcall ABI mismatch in shift-amount argument

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4001
+        EVT::getIntegerVT(*DAG.getContext(), DAG.getLibInfo().getIntSize());
+    SDValue ShAmt = DAG.getAnyExtOrTrunc(N->getOperand(1), dl, ShAmtTy);
+    SDValue Ops[2] = {N->getOperand(0), ShAmt};
----------------
nextsilicon-itay-bookstein wrote:
> bjope wrote:
> > AnyExt seem a bit weird as I assume the lib function will use all bits. I think getZExtOrTrunc would be a more correct choice here.
> I agree that AnyExt is incorrect, however I'm wondering whether it should be ZExt or SExt. The source-level semantics seem to point towards SExt, because the libcalls are defined with a signed shift amount (`si_int` as opposed to `su_int`). Earlier code in this function uses ZExt (under the `LegalOrCustom && TLI.shouldExpandShift(DAG, N)` case), but it doesn't need to concern itself with the signature for `__ashlti3` and friends. Note that this actually affects the output for `llvm/test/CodeGen/X86/shift_minsize.ll` functions `shl128, ashr128, lshr128`, in whether the `movzbl` is emitted or not.
My reasoning was that the shift amount can't be negative (that would be undefined both for the SelectionDAG nodes and afaik the lib calls (e.g. __ashlti3 expects the shift amount `b` to be in the range `0 <= b < "bits in a tword"`. And at least for rotates and funnel shifts the shift amount (in SelectionDAG nodes) is treated as an unsigned. So I kind of assumed that doing a zero extend would be safe, as it would avoid passing a negative shift amount to the lib function (e.g. if the shift amount is 15 and the type we should convert from is i4, then doing a zext to a 32 bit si_int (treated as signed) would still give the value 15, while doing a sext would result in passing -1 to the lib function).


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

https://reviews.llvm.org/D110508