Page MenuHomePhabricator

[InstCombine] Skip scalable vectors in combineLoadToOperationType
ClosedPublic

Authored by rovka on Nov 11 2019, 5:01 AM.

Details

Summary

Don't try to canonicalize loads to scalable vector types to loads
of integers.

This removes one assertion when trying to use a TypeSize as a parameter
to DataLayout::isLegalInteger. It does not handle the second part of the
function (which looks at bitcasts).

This patch also contains a NFC fix for Load Analysis, where a variable
initialization that would cause the same assertion is moved closer to
its use. This allows us to run the new test for InstCombine without
having to teach LocationSize to play nicely with scalable vectors.

Diff Detail

Event Timeline

rovka created this revision.Nov 11 2019, 5:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2019, 5:01 AM

Thanks for this fix @rovka!

llvm/test/Transforms/InstCombine/load.ll
240

Is this just adding a missing test-case for existing code, or are you trying to test something else?

340

Does this need a CHECK line to test for non-assert builds?

rovka marked 2 inline comments as done.Nov 12 2019, 1:31 AM
rovka added inline comments.
llvm/test/Transforms/InstCombine/load.ll
240

I'm just trying to make sure we're not accidentally breaking anything for fixed-sized vectors. It's essentially the same test-case as test16, but with vectors instead of float (the code should do the same thing for any type of that size). It should work the same before and after this patch, so I could commit it separately if you think it makes more sense that way. Personally I think it fits well in the context of this patch, since we're not trying to exhaustively test every possible type.

340

Oops! Of course it does, I just forgot to re-run update_test_checks after getting it to pass. Thanks for catching it.

rovka updated this revision to Diff 228827.Nov 12 2019, 1:33 AM

Update test checks. Also brush it up a bit so it fits better with the rest of the tests in the file.

sdesmalen accepted this revision.Nov 12 2019, 1:47 AM

LGTM!

llvm/test/Transforms/InstCombine/load.ll
240

Okay that's fine, I agree we can keep the test. Thanks for clarifying!

This revision is now accepted and ready to land.Nov 12 2019, 1:47 AM
This revision was automatically updated to reflect the committed changes.