This is an archive of the discontinued LLVM Phabricator instance.

[SVE][IR] Scalable Vector IR Type with pr42210 fix
ClosedPublic

Authored by huntergr on Jun 14 2019, 12:56 AM.

Details

Summary

Same as D32530 except for a (much) lighter-weight method of rejecting scalable vectors in globals, arrays, and structs in the verifier and a couple of comment changes.

Should address pr42210, which had problems with compile time using LTO due to the recursive verifier implementation.

Diff Detail

Event Timeline

huntergr created this revision.Jun 14 2019, 12:56 AM
Ka-Ka added a subscriber: Ka-Ka.Jun 14 2019, 3:28 AM
thakis added inline comments.Jun 14 2019, 9:34 AM
lib/IR/Verifier.cpp
343

That's still a lot of type walking. Did you have a chance to measure time to thinlto-link some large-ish target with and without this change?

thakis added inline comments.Jun 14 2019, 11:25 AM
lib/IR/Verifier.cpp
343

Just saw your comment on https://bugs.llvm.org/show_bug.cgi?id=42210 . That seems fine to me. (I usually do min-of-3 when timing things, to reduce effects of caches.) So from my point of view this is ready for relanding.

huntergr marked an inline comment as done.Jun 17 2019, 3:17 AM
huntergr added inline comments.
lib/IR/Verifier.cpp
343

Yeah, I ran them 5 times each (and cleared the thinlto cache directory between each run, though not the linux fs cache).

The main difference is that I'm not recursing into aggregates-within-aggregates; if there's aggregates with scalable types nested in arrays or structs, they'll still be picked up by the (mostly) linear walks. The fully recursive approach was a leftover from when we only rejected scalable vectors from global variables.

sdesmalen accepted this revision.Jun 17 2019, 4:07 AM

Removing the recursion solves the compile-time issue and still catches all invalid cases.
LGTM.

test/Verifier/scalable-aggregates.ll
7

Just pointing out that the Verifier still errors on all the same invalid cases, but now only the reports errors once compared to the previous patch.

This revision is now accepted and ready to land.Jun 17 2019, 4:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2019, 3:10 AM
Herald added a subscriber: kristina. · View Herald Transcript