[PATCH] D80262: [SVE] Eliminate bad VectorType::getNumElements() calls from ConstantFold

[Sander de Smalen via Phabricator via llvm-commits]

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

LGTM.



================
Comment at: llvm/lib/IR/ConstantFold.cpp:812
 
+  auto *ValVTy = cast<FixedVectorType>(Val->getType());
+
----------------
ctetreau wrote:
> ctetreau wrote:
> > sdesmalen wrote:
> > > How about `extractelement <vscale x 16 x i8> zeroinitializer, i64 0` ?
> > > 
> > > (This probably cannot be a Constant anymore when the index > 16)
> > I don't quite remember what (if anything) we decided to do about this case last thursday. I believe I was on the "it might be out of bounds, but we can't know so we shouldn't do anything" side of the argument, but I don't remember which side won.
> > 
> > Regardless, it would probably be more appropriate to just skip this particular transformation rather than letting it explode. I think it makes sense to just do that for now and revisit when we have all the correctness issues sorted out. I suppose this represents a behavior change, and needs a test...
> I went ahead and explicitly added support for this case when the index is less than the minNumElements. 
> I went ahead and explicitly added support for this case when the index is less than the minNumElements
Thanks, I think that's the right fix.

This change probably sufficient, because AIUI there is no issue with this form if index > 16. The LangRef indeed specifies the result is 'Poison' if this turns out to be out-of-bounds, so that will lead to undefined behaviour in its propagated uses, there just isn't necessarily a way to know that it is poison at compiletime. The ISD nodes have comments saying that this case should be guarded so that it doesn't cause a runcrash at the point of doing the extract (e.g.if the extract is implemented by doing a vector store + scalar load). This behaviour is not really any different from if it wasn't constant.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:1374
     }
-  } else if (VectorType *VTy = dyn_cast<VectorType>(C1->getType())) {
+  } else if (IsScalableVector) {
     // Do not iterate on scalable vector. The number of elements is unknown at
----------------
ctetreau wrote:
> sdesmalen wrote:
> > Although not as superfluous as the case above, I still think it can be removed.
> This could potentially result in a behavior change, which I am trying to avoid as much as possible in this patch. If you're sure it doesn't, I can change it. Otherwise, I can add a FIXME.
maybe better to err on the safe side here. Fine to leave it as is.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:2294
+      if (VectorType *VT = dyn_cast<VectorType>(C->getType())) {
+        // FIXME: handle scalable vectors
+        GEPTy = FixedVectorType::get(
----------------
ctetreau wrote:
> sdesmalen wrote:
> > Is it worth just fixing this for scalable vectors instead of adding the FIXME and then doing the wrong thing? It is possible to construct a test-case that breaks this assert.
> > 
> > I think you can use `VectorType::get(..., VT->getElementCount())` here (but it would need a test-case)
> > 
> > 
> If I had the authority to make the call, I'd say we should just replace all instances of `VectorType::get(..., SomeVec->getNumElements() /*, false */)` with `VectorType::get(..., SomeVec->getElementCount())` without tests, as the original form is almost certainly a bug.
> 
> Unfortunately, others disagree with this.  While it pains me to write this code, my current goal is to just get rid of the bad usages of VectorType::getNumElements in as frictionless a manner as possible, which means taking the coward's way out so as not to get bogged down writing a billion tests.
I see your point, but there's the danger that without tests we don't guard the code-path now working for scalable vectors . If you make the change to getElementCount, we'll probably never revisit this to make a test for it. If we add tests while going through the code-base, we'll incrementally guard other code-paths as well, because it's difficult to test these things entirely in isolation.

I tried constructing a test for this case, but this leads to an unrelated issue parsing the IR, `error: constant expression type mismatch`.
```define <vscale x 2 x i64*> @foo() {
  ret <vscale x 2 x i64*> getelementptr ([1 x i64], [1 x i64]* null, i64 0, <vscale x 2 x i64> zeroinitializer)
}```
whereas removing `vscale x` causes the expression to be folded.

I agree that this (together with parser error) is better fixed in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80262