This is an archive of the discontinued LLVM Phabricator instance.

[LoopInterchange] Ignore debug intrinsics during legality checks.
ClosedPublic

Authored by fhahn on Apr 6 2018, 10:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Apr 6 2018, 10:59 AM

This is generally great, I think the helper function would be useful outside of this source file, too.

lib/Transforms/Scalar/LoopInterchange.cpp
82 ↗(On Diff #141379)

This adapter should really be available in BasicBlock.h or somewhere more accessible if it doesn't exist already. Could you move it there (or elsewhere appropriate) and add a doxygen comment?

1000 ↗(On Diff #141379)

I'm assuming that dbg intrinsic calls don't read memory, so this change may be a noop. Feel free to keep it for readability though.

vsk added a subscriber: vsk.Apr 6 2018, 3:57 PM
vsk added inline comments.
lib/Transforms/Scalar/LoopInterchange.cpp
82 ↗(On Diff #141379)

+ 1, I think having iterator_range<iterator> instructions() and ... instructions_nodbg() in BasicBlock would be nice.

fhahn added inline comments.Apr 9 2018, 11:32 AM
lib/Transforms/Scalar/LoopInterchange.cpp
82 ↗(On Diff #141379)

Ok great, I will move it there. Do you think we should just have a member function instructionsNodbg() or a function taking an instruction iterator and returning a filtered iterator? It's just a question how we want to compose it with things like reverse

vsk added inline comments.Apr 9 2018, 11:42 AM
lib/Transforms/Scalar/LoopInterchange.cpp
82 ↗(On Diff #141379)

I think we should have both. We'd need 'skipDebugIntrinsics' to implement 'instructionsNodbg', and 'instructionsNodbg' would make it easier to write correct code going forwards.

Wdyt of placing 'skipDebugIntrinsics' in IR/DebugInfo.h?

mattd added a subscriber: mattd.Apr 9 2018, 11:56 AM
mattd added inline comments.
lib/Transforms/Scalar/LoopInterchange.cpp
82 ↗(On Diff #141379)

I'm a bit late to the party here, but have been keeping an eye on this discussion. I really like this idea. What about exposing this in IR/Intrinisics.h instead? For instance, what if there were other intrinsics that we would like to skip over as well (e.g., lifetime.start, lifetime.end). In that case I can imagine (in the future) an iterator that takes a list of types to skip-over, like a parameter pack or something, although I'm not sure if that would work with make_filter_range, just a thought.

fhahn added inline comments.Apr 10 2018, 3:00 PM
lib/Transforms/Scalar/LoopInterchange.cpp
82 ↗(On Diff #141379)

Having an easy way to skip all kinds of instructions sounds like a great idea. I will try to put initial patches together in the next few days.

aprantl accepted this revision.Apr 10 2018, 6:39 PM

LGTM with whatever new API for skipping we come up with.

This revision is now accepted and ready to land.Apr 10 2018, 6:39 PM
fhahn updated this revision to Diff 144077.Apr 26 2018, 2:20 AM
fhahn edited the summary of this revision. (Show Details)

Rebased after rL330875 went in

This revision was automatically updated to reflect the committed changes.