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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli accepted this revision.
fpetrogalli added a comment.
This revision is now accepted and ready to land.

LGTM, please consider adding those two comments I mentioned in my last review before submitting.

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
----------------
david-arm wrote:
> fpetrogalli wrote:
> > 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.
> Hi Francesco, I'm sorry but I don't quite follow here. All the code below this is not custom code for scalable vectors. Even if we later fixed the code below to work for both fixed and scalable, it still won't be specific to scalable.
I wanted to make sure that there was a clear cut between the code for scalable vectors and the code for fixed width vector. So maybe it makes sense to leave the comment as it is, and just modify line 4952 to say:

```
// For fixed width vectors, extract each constant element and fold them individually.
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5035
+
   unsigned NumElts = VT.getVectorNumElements();
 
----------------
Same here, add a comment saying that from now on the vectors are assumed to be fixed width.


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

https://reviews.llvm.org/D79421