[PATCH] D103180: [InstSimplify] Add constant fold for extractelement + splat for scalable vectors

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.

The patch looks OK to me. I added some inline comments but you don't need to do anything about them, they are just observations.



================
Comment at: llvm/lib/IR/ConstantFold.cpp:912
+  //  extractelt Splat(x), n -> x
   if (auto *ValSVTy = dyn_cast<ScalableVectorType>(Val->getType())) {
+    if (CIdx->getValue().ult(ValSVTy->getMinNumElements())) {
----------------
I don't really understand why this special case for scalable types exists here. Why not just fall through to the call to `Val->getAggregateElement` below? It looks like getAggregateElement already has some support for scalable types.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:913
   if (auto *ValSVTy = dyn_cast<ScalableVectorType>(Val->getType())) {
     if (!CIdx->uge(ValSVTy->getMinNumElements())) {
+      if (Constant *SplatVal = Val->getSplatValue())
----------------
sdesmalen wrote:
> dmgreen wrote:
> > sdesmalen wrote:
> > > CarolineConcatto wrote:
> > > > sdesmalen wrote:
> > > > > nit: `s/!CIdx->uge/CIdx->ult/`
> > > > Hey @sdesmalen 
> > > > I may be completely wrong, but I believe your suggestion is not possible
> > > > https://llvm.org/doxygen/classllvm_1_1ConstantInt.html
> > > > If I looked the documentation correctly llvm::constant only has UGE
> > > Ah I hadn't realised that. In that case, could you write it more explicitly as
> > >   CIdx->getZExtValue() < ValSVTy->getMinNumElements()
> > Do we know the type of CIdx is always less that 64bits?
> > 
> > uge() is just a convenience wrapper around APInt::uge(). You can use CIdx->getValue().ult(..) if you feel the ult is better than the !uge
> Agreed, that's better, my main concern was the `!uge`.
Or you could implement ConstantInt::ult.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103180