This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Unary vector constant folding integer legality fixes
ClosedPublic

Authored by RKSimon on Apr 26 2015, 7:28 AM.

Details

Summary

This patch fixes issues with vector constant folding not correctly handling scalar input operands if they require implicit truncation - this was tested with llvm-stress as recommended by Patrik.

The patch ensures that integer input scalars from a build vector are correctly truncated before folding, and that constant integer scalar results are promoted to a legal type before inclusion in the new folded build vector. I've ran llvm-stress for extended lengths of time without any issue due to the vector constant folding path.

I have added another crash test case and also a test for UINT_TO_FP / SINT_TO_FP using an non-truncated scalar input, which was failing before this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 24443.Apr 26 2015, 7:28 AM
RKSimon retitled this revision from to [SelectionDAG] Unary vector constant folding integer legality fixes.
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
silvas added a subscriber: silvas.Apr 27 2015, 1:32 PM

We probably don't want to directly commit the test case generated by llvm-stress. Can you reduce it? These are theoretically the sorts of things that bugpoint should reduce really well, although I've always found it hard to use.

http://llvm.org/docs/CommandGuide/bugpoint.html
http://llvm.org/docs/Bugpoint.html

chfast added a subscriber: chfast.Apr 27 2015, 2:40 PM
RKSimon updated this revision to Diff 24505.Apr 27 2015, 3:00 PM

Reduced the llvm-stress crash case with bugpoint - thanks for the reminder Sean.

ab added a subscriber: ab.Apr 28 2015, 11:54 AM

Is there a way to further reduce the testcase, or even rewrite it from scratch? I shiver whenever I see autogen ;)

The change LGTM, but you might want to wait for other reviews.

test/CodeGen/X86/fold-vector-bv-crash.ll
1–2 ↗(On Diff #24505)

cpu -> attr?

test/CodeGen/X86/fold-vector-trunc-sitofp.ll
1–2 ↗(On Diff #24505)

Isn't SSE2 enough?

RKSimon updated this revision to Diff 24576.Apr 28 2015, 2:03 PM

Cleaned up crash test (to remove the autogen stink!)

In D9282#162777, @ab wrote:

Is there a way to further reduce the testcase, or even rewrite it from scratch? I shiver whenever I see autogen ;)

The change LGTM, but you might want to wait for other reviews.

Thanks Ahmed, I'll give it another day to see if anyone else has any comments.

I'll get the test -mcpu fixed - note we need mattr=+avx to force i686 to use xmm registers, otherwise it does a rather messy return of the <4 x float> over the stack which isn't very helpful.

This revision was automatically updated to reflect the committed changes.