Page MenuHomePhabricator

Do not assume that FP vector operands are never legalized by expanding
ClosedPublic

Authored by nemanjai on Oct 17 2016, 7:49 AM.

Details

Summary

This patch ensures that if a floating point vector operand is legalized by expanding, it is legalized through the stack rather than by calling DAGTypeLegalizer::IntegerToVector which will cause a failure since the operand is a non-integer type.

This fixes PR 30715.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 74847.Oct 17 2016, 7:49 AM
nemanjai retitled this revision from to Do not assume that FP vector operands are never legalized by expanding.
nemanjai updated this object.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: llvm-commits.
amehsan added inline comments.Oct 17 2016, 9:34 AM
lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
337–340

So, for cases that your new condition is not satisfied, what change do we make in the generated code? (The question applies to other platforms as well). I believe there is no testcase in unit-tests for this, (otherwise it would have failed and would have been included in your changes). Still this may happen in benchmarks and real work loads. I think you may need to investigate how that code pattern changes and what is the potential performance impact of your change for that code pattern.

test/CodeGen/PowerPC/pr30715.ll
8

Please remove mangled name from the test file.

amehsan added inline comments.Oct 17 2016, 9:39 AM
lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
337–340

I am asking because this function is called from places like DAGTypeLegalizer::ExpandFloatOperand. My question is the fact that you get a failure for floating point case, is applicable to all platforms or not?

test/CodeGen/PowerPC/pr30715.ll
8

Also limit the lines to 80 characters

nemanjai added inline comments.Oct 17 2016, 4:17 PM
lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
337–340

If you expand the context in the diff a bit, you'll see where the assert that we trip on is located. Namely, line 321. If this function was ever called with a node whose result is a vector and first operand was a floating point type, that assert would have tripped. All this patch changes is that rather than ignoring the possibility that the operand is a non-integer type and then failing at the assert, we check for this and don't call IntegerToVector.

So given that this conditional block could not possibly succeed if the result is a vector and the operand is non-integer, I don't see how we can investigate the performance impact of this change. Namely, cases where we would enter this block before and we don't now are exactly those that would not result in a successful compilation before. So the performance difference between the code we produce now and the code we would produce without this patch cannot be studied.

And I think this makes sense because for example, we can easily expand something like:

tX; v2i64 = bitcast tY
  tY: i128 = ...

by splitting the i128 value and passing the two i64 values to a BUILD_VECTOR node.
However, doing something similar to the equivalent FP node would require adding some machinery that I'm not sure would be worth the effort. Namely, handling this:

tX: v2f64 = bitcast tY
  t81: ppcf128 = ...

By splitting and using BUILD_VECTOR vs. through the stack is not something that is likely to come up frequently enough in real code that it would warrant the additional machinery. And of course, doing the same thing for f128 would be significantly more tricky (I don't know enough about IEEE binary-128 to know if it's even possible). So I believe that going through the stack here is the right thing to do.
Finally, even if we find some code where this happens a lot for ppcf128, I am still not convinced that we would gain much by avoiding the stack since the actual operations on the type are actually done in software, so it is unlikely that this code is expecting particularly high performance. But that's just my opinion.

test/CodeGen/PowerPC/pr30715.ll
8

OK, I'll remove the mangled name. But I don't think we generally limit the IR in test cases to 80 characters - it's whatever Clang produces for the original test case.

amehsan added inline comments.Oct 17 2016, 4:22 PM
lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
337–340

Yes, if the assert is inside this conditional this is fine. I did look at the code, but it was not quite obvious, or maybe I missed it.

kbarton accepted this revision.Oct 20 2016, 12:36 PM
kbarton edited edge metadata.

LGTM
However, I'm not familiar with this logic here so someone else should take a look also to verify this change is OK. It basically assumes that the ExpandOp_BITCAST should be called on a vector of integers; if it is a vector of any other type there there will be no effect. @hfinkel or @tstellarAMD, would you be able to take a quick look? Thanks.

This revision is now accepted and ready to land.Oct 20 2016, 12:36 PM

As far as I can tell, ExpandOp_BITCAST can be called with two kinds of nodes: a bitcast where the operand is a large illegal integer type (DAGTypeLegalizer::ExpandIntegerOperand), and a bitcast where the operand is of type ppc_fp128 (DAGTypeLegalizer::ExpandFloatOperand; the name is slightly misleading, but it is in fact only used for ppc_fp128). So this should have no effect anywhere outside of PPC targets.

Given that, it's probably a good idea to reorganize the code so it's less likely someone will break ppc_fp128 in the future... but the bugfix is fine.

nemanjai closed this revision.Oct 26 2016, 1:01 PM

Committed revision 285231.