[PATCH] D79421: [CodeGen] Fix FoldConstantVectorArithmetic for scalable vectors

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

> The code path I have changed is defended by this existing test:
> 
>   CodeGen/AArch64/sve-int-arith.ll

Hi @david-arm ,

NIT1: in what way does the test defend the change? The test is successful with and without your changes, so I think this comment in the commit message is misleading I know you have added this to remove the warnings generated when using getVectorNumElements on scalable vectors, so my personal preference is that there is nothing to defend here, it is just a sensible change to do to avoid entering code that makes invalid assumption on the underlying VTs. I think you can remove the paragraph I quoted from the commit message.

Grazie!

Francesco



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4946
 
+  // TODO: All the folds below are performed lane-by-lane and assume a fixed
+  // vector width, however we should be able to do constant folds involving
----------------
NIT2: In the spirit of NIT1, I think you should add the TODO only on the part that requires implementing constant folding for splat into scalable vectors, as follows:

```
// Custom code for scalable vectors, as all the folds below assume a fixed width vector.
// TODO: bail off for now, however we should be able to do constant folds involving splats to scalable vectors too.
``` 

This way, once the TODO is done and removed, we will not loose the information on why the scalable vectors require custom code.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5029-5031
+  // TODO: All the folds below are performed lane-by-lane and assume a fixed
+  // vector width, however we should be able to do constant folds involving
+  // splat vector nodes too.
----------------
Ditto.


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

https://reviews.llvm.org/D79421