This is an archive of the discontinued LLVM Phabricator instance.

Fix bug in DAGBuilder for getelementptr with expanded vector.
ClosedPublic

Authored by aymanmus on Aug 9 2016, 3:52 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

aymanmus updated this revision to Diff 67314.Aug 9 2016, 3:52 AM
aymanmus retitled this revision from to Fix bug in DAGBuilder for getelementptr with expanded vector..
aymanmus updated this object.
aymanmus added reviewers: delena, mkuper.
aymanmus added a subscriber: llvm-commits.
mkuper added inline comments.Aug 9 2016, 10:09 AM
test/CodeGen/X86/gep-expanded-vector.ll
1 ↗(On Diff #67314)

Why does this test require SKX? If we're crashing in the builder - because v64i64 isn't simple - this should crash regardless of feature-set. If you do need AVX512 of some sort, it would be better to specify it explicitly with -mattr.
On the other hand, you probably want to specify a triple - the issue is x86_64-only, right?

7 ↗(On Diff #67314)

Can you modify the test so that it (a) produces some kind of result, and (b) we check that we get the right result (as opposed to just "not crashing")?

aymanmus updated this revision to Diff 67973.Aug 14 2016, 1:30 AM

Updating the test to actually check that code is emitted with right address calculation.

mkuper edited edge metadata.Aug 14 2016, 4:24 PM

The test looks better now - but, as I wrote on the last iteration, please specify a triple (either as part of the IR, or, better yet, pass -mtriple to llc).
Otherwise, the test is dependent on the host triple - so you'll break every build that's not x86_64.

aymanmus updated this revision to Diff 67997.Aug 14 2016, 11:50 PM
aymanmus edited edge metadata.

Specifying triple for test llc run.

mkuper accepted this revision.Aug 15 2016, 12:10 AM
mkuper edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 15 2016, 12:10 AM
This revision was automatically updated to reflect the committed changes.