Page MenuHomePhabricator

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

Authored by ctetreau on May 19 2020, 5:24 PM.

Details

Summary

Assume all usages of this function are explicitly fixed-width operations
and cast to FixedVectorType

Diff Detail

Event Timeline

ctetreau created this revision.May 19 2020, 5:24 PM
Herald added a project: Restricted Project. · View Herald Transcript
sdesmalen added inline comments.Jun 9 2020, 10:00 AM
llvm/lib/IR/ConstantFold.cpp
574

The FIXME is probably unnecessary as it should never be possible to bitcast a fixed-width vector to a scalable vector.

813

How about extractelement <vscale x 16 x i8> zeroinitializer, i64 0 ?

(This probably cannot be a Constant anymore when the index > 16)

1014–1015

This condition/branch has become superfluous given the use of dyn_cast<FixedVectorType> below, and the default of return nullptr.

1381

Although not as superfluous as the case above, I still think it can be removed.

2302

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)

ctetreau updated this revision to Diff 270854.Jun 15 2020, 1:55 PM
ctetreau marked 4 inline comments as done.

address code review issues

ctetreau added inline comments.Jun 15 2020, 2:07 PM
llvm/lib/IR/ConstantFold.cpp
813

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...

1014–1015

good catch

1381

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.

2302

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.

ctetreau updated this revision to Diff 270885.Jun 15 2020, 3:25 PM

Add support and test for for constant folding extractelement (<vscale x n x ty> zeroinitializer, i64 i) for i = 0 .. n - 1

ctetreau marked 3 inline comments as done.Jun 15 2020, 3:29 PM
ctetreau added inline comments.
llvm/lib/IR/ConstantFold.cpp
813

I went ahead and explicitly added support for this case when the index is less than the minNumElements.

sdesmalen accepted this revision.Jun 17 2020, 9:38 AM

LGTM.

llvm/lib/IR/ConstantFold.cpp
813

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.

1381

maybe better to err on the safe side here. Fine to leave it as is.

2302

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.

This revision is now accepted and ready to land.Jun 17 2020, 9:38 AM
This revision was automatically updated to reflect the committed changes.