This is an archive of the discontinued LLVM Phabricator instance.

[LAA] Move runtime-check generation to Transforms/Utils/loopUtils (NFC)
ClosedPublic

Authored by fhahn on Apr 19 2020, 12:56 PM.

Details

Summary

Currently LAA's uses of ScalarEvolutionExpander blocks moving the
expander from Analysis to Transforms. Conceptually the expander does not
fit into Analysis (it is only used for code generation) and
runtime-check generation also seems to be better suited as a
transformation utility.

Diff Detail

Event Timeline

fhahn created this revision.Apr 19 2020, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2020, 12:56 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn updated this revision to Diff 260880.Apr 29 2020, 4:21 AM

Ping. Rebased after 616657b39c81

Ayal added a comment.May 9 2020, 6:10 AM

This analysis/utils refactoring looks good to me, thanks! Just adding some nits to clarify some presumably "add-on" parts, and possible alternatives/follow-ups.

llvm/include/llvm/Transforms/Utils/LoopUtils.h
50

nit: may be worthwhile to have a typdef for SmallVectorImpl<RuntimePointerCheck> as well.

437

nit: would be good to retain some of the original explanation of the abandoned addRuntimeChecks():

/// Add code that checks at runtime if the accessed arrays overlap.
llvm/lib/Transforms/Utils/LoopUtils.cpp
1556

nit: is there a reason to swap the order of getFirstInst() and struct PointerBounds?

1563

Just to clarify: using CG->ReCheck to save passing PtrRtChecking parameter here and through the other expandBounds() could have been done as a preparatory patch, right?

Seems like CG should provide an API for retrieving the desired Ptr of its first member.

llvm/lib/Transforms/Utils/LoopVersioning.cpp
65

Seems strange to call getRuntimePointerChecking() only for its getSE(). Perhaps pass RtPtrChecking, as commented below for LV, and have addRuntimeChecks() retrieve both AliasChecks and SE from there. This would require modifying the way (not) UseLAIChecks is handled.

Moreover, perhaps LoopVersioning's additional use of AliasChecks to annotate insns with no alias, should be shared with LV, or in general be taken care of by addRuntimeChecks()?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2795

Just to clarify: inlining addRuntimeChecks(Insn)'s call to RtPtrChecking.Need and discarding the former could have been done as a preparatory patch, right?

OTOH, instead of passing both RtPtrChecking.getChecks() and RtPtrChecking.getSE(), and potentially also RtPtrChecking.Need, perhaps pass only RtPtrChecking. This early-exit check for Need could also apply to LoopVersioning, right?

fhahn updated this revision to Diff 263040.May 9 2020, 2:31 PM
fhahn marked 6 inline comments as done.

Thanks Ayal! Comments should be addressed.

llvm/include/llvm/Transforms/Utils/LoopUtils.h
437

I've reworded the first line with the more descriptive message, thanks!

llvm/lib/Transforms/Utils/LoopUtils.cpp
1556

I've moved it into a lambda in the single function using it.

1563

Just to clarify: using CG->ReCheck to save passing PtrRtChecking parameter here and through the other expandBounds() could have been done as a preparatory patch, right?

I've pushed 57fb56b30e85c8e9662075c671d02fbdc37d8f3b doing exactly that, thanks.

Seems like CG should provide an API for retrieving the desired Ptr of its first member.

Can do that as a follow up.

llvm/lib/Transforms/Utils/LoopVersioning.cpp
65

Seems strange to call getRuntimePointerChecking() only for its getSE(). Perhaps pass RtPtrChecking, as commented below for LV, and have addRuntimeChecks() retrieve both AliasChecks and SE from there. This would require modifying the way (not) UseLAIChecks is handled.

I tried that, but it seems the LoopVersioning stores the checks on purpose. When using getChecks() directly, there are some test failures, although I did not spot the places that changes the checks on a quick look. I think unifying them would be better as a follow-up patch, if possible.

Moreover, perhaps LoopVersioning's additional use of AliasChecks to annotate insns with no alias, should be shared with LV, or in general be taken care of by addRuntimeChecks()?

+1. Not sure if there is a reason for the split, but I can take a look as a follow-up/

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2795

Just to clarify: inlining addRuntimeChecks(Insn)'s call to RtPtrChecking.Need and discarding the former could have been done as a preparatory patch, right?

Done, I've put up D79679

OTOH, instead of passing both RtPtrChecking.getChecks() and RtPtrChecking.getSE(), and potentially also RtPtrChecking.Need, perhaps pass only RtPtrChecking. This early-exit check for Need could also apply to LoopVersioning, right?

It looks like there's an issue with passing RtPtrchecking directly for LoopVersioning. I can look into what is going on there, but I think it would be better as follow-up, because it seems a bit more risky.

Ayal accepted this revision.May 9 2020, 3:58 PM

This looks good to me, thanks!
Noting a minor nit.
Follow-ups could be noted in TODO's.

llvm/lib/Transforms/Utils/LoopUtils.cpp
1628

Is this the indentation of this lambda right?

This revision is now accepted and ready to land.May 9 2020, 3:58 PM
fhahn updated this revision to Diff 263071.May 10 2020, 8:47 AM

Thanks Ayal, I fixed the formatting and added the TODOs

This revision was automatically updated to reflect the committed changes.