[PATCH] D68203: Add support for (expressing) vscale.

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 14 11:03:04 PST 2020


efriedma added a comment.

LGTM.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5139
+      int64_t MulImm = cast<ConstantSDNode>(N1->getOperand(0))->getSExtValue();
+      return getVScale(DL, VT, MulImm * N2C->getSExtValue());
+    }
----------------
sdesmalen wrote:
> efriedma wrote:
> > efriedma wrote:
> > > This multiply can overflow.
> > This multiply can still overflow.  (If it's supposed to wrap, please use an unsigned multiply.)
> Sorry, I'm not really sure I understand how the multiply above can overflow. The code checks the NSW flag on the MUL node, so if `VSCALE(Imm) *N2C` does not wrap, why would `Imm * N2C` wrap?
> 
> Perhaps int64_t is the wrong choice here though and we should instead be using APInt for the VScale operand (for the general case where VT > int64_t)
APInt is fine.

In the version you had, the multiply "can't overflow" in the sense that the operation produces poison if it overflows; that's not a problem.  The problem would be undefined behavior in the compiler itself.


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

https://reviews.llvm.org/D68203





More information about the llvm-commits mailing list