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

[Eli Friedman via Phabricator via llvm-commits]

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