[PATCH] D56987: [Intrinsic] Expand vector SMULFIX to MUL on zero scale

Leonard Chan via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 22 11:58:19 PST 2019


leonardchan added a comment.

In D56987#1364931 <https://reviews.llvm.org/D56987#1364931>, @bjope wrote:

> How common would it be that the scale is zero? Is that really expected in reality or just in this kind of handwritten test cases?


It's not as expected or important as with a non-zero scale, but it seems like a good case to cover. It's mostly just another test case to cover.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5373
+  if (VT.isVector())
+    // Do not scalarize here.
+    return SDValue();
----------------
bjope wrote:
> nit: This comment is a little bit confusing (does "here" refer to this method or the context for the legalization). Returning SDValue() will result in an unroll, so by doing that I think we will end up scalarizing the mulfix operation. I'd expect a comment explaining that we return SDValue() to get scalarization, but now the comment could be read as "avoid scalarization by returning SDValue()".
> 
> Maybe you can change the wording somehow, or simply remove the comment.
Removed the comment


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5397
     Hi = DAG.getNode(ISD::MULHS, dl, VT, LHS, RHS);
   } else {
     report_fatal_error("Unable to expand signed fixed point multiplication.");
----------------
RKSimon wrote:
> Why not let vectors try to use these cases and add a bail out?
> ```
> } else if (VT.isVector()) {
>   // Only expand vector types if we have the appropriate vector operations.
>   return SDValue();
> } else {
> ```
Done. This also produces fewer instructions for the vector case with non-zero scale.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56987





More information about the llvm-commits mailing list