Page MenuHomePhabricator

Making Instrumentation aware of LoopNest Pass
ClosedPublic

Authored by raghesh on May 13 2021, 9:44 PM.

Details

Summary

Intrumentation callbacks are not made aware of LoopNest passes. From the loop pass manager, we can pass the outermost loop of the LoopNest to instrumentation in case of LoopNest passes.

The current patch made the change in two places in StandardInstrumentation.cpp. I will submit a proper patch where the OuterMostLoop is passed from the LoopPassManager to the call backs. That way we will avoid making changes at multiple places in StandardInstrumentation.cpp.

A testcase also will be submitted.

Diff Detail

Event Timeline

raghesh created this revision.May 13 2021, 9:44 PM
raghesh requested review of this revision.May 13 2021, 9:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 9:44 PM
raghesh edited the summary of this revision. (Show Details)May 13 2021, 9:46 PM

I thought we were going to make changes to always pass the Loop (as mentioned in the description), and not do this patch

I thought we were going to make changes to always pass the Loop (as mentioned in the description), and not do this patch

Yes. I would like to amend this patch in such a way finally as mentioned in the description. Created this so that I don't miss to follow it up.

raghesh updated this revision to Diff 345754.May 16 2021, 11:25 PM

Passing the outermost loop from LoopPassManager to the call backs in case of LoopNest pass.

do you have a test case? probably a super simple one that crashes without this patch is good enough
it probably goes somewhere in llvm/test/Other/

aeubanks added inline comments.May 17 2021, 12:52 PM
llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
369–371

IR is either a Loop& or a LoopNest& right? not an Any
so we need to extract a Loop out of either Loop or LoopNest. with templates, it should be as simple as creating two functions with the same name that both return a Loop&, where one takes Loop& and the other takes a LoopNest&

raghesh added inline comments.May 18 2021, 7:23 AM
llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
369–371

Sorry!! I dont really get you. Are you suggesting something similar to the following

@@ -183,6 +183,9 @@ protected:

PreservedAnalyses runWithoutLoopNestPasses(Loop &L, LoopAnalysisManager &AM,
                                           LoopStandardAnalysisResults &AR,
                                           LPMUpdater &U);

+private:
+ const Loop& getLoopFromIR(Loop &L) { return L; }
+ const Loop& getLoopFromIR(LoopNest &LN) { return LN.getOutermostLoop(); }
};

/// The Loop pass manager.
@@ -360,12 +363,10 @@ Optional<PreservedAnalyses> LoopPassManager::runSinglePass(

  LoopStandardAnalysisResults &AR, LPMUpdater &U, PassInstrumentation &PI) {
// Pass the outermost loop to BeforePass and AfterPass callbacks in case of
// LoopNest Pass.

+ const Loop &L = getLoopFromIR(IR);

// Check the PassInstrumentation's BeforePass callbacks before running the
// pass, skip its execution completely if asked to (callback returns false).
  • if (!PI.runBeforePass<Loop>(*Pass, *L))

+ if (!PI.runBeforePass<Loop>(*Pass, L))

  return None;

PreservedAnalyses PA;

@@ -378,7 +379,7 @@ Optional<PreservedAnalyses> LoopPassManager::runSinglePass(

if (U.skipCurrentLoop())
  PI.runAfterPassInvalidated<IRUnitT>(*Pass, PA);
else
  • PI.runAfterPass<Loop>(*Pass, *L, PA);

+ PI.runAfterPass<Loop>(*Pass, L, PA);

return PA;

}

aeubanks added inline comments.May 18 2021, 9:00 AM
llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
369–371

yup that's right

I don't think the current patch does anything because there's no Any in this code (a test case would be nice)

raghesh updated this revision to Diff 346739.May 20 2021, 8:00 AM

Made the suggested change. Added a simple testcase which uses -print-after for LoopInterchange pass (a LoopNest pass). This would crash without this patch.

aeubanks added inline comments.May 21 2021, 10:44 AM
llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
188

these probably make more sense as static functions, but not a huge deal

llvm/test/Other/loopnest-callback.ll
2

-passes=loop-interchange

3

--print-after-all, --print-after isn't super well supported in the new PM due to pass naming being weird

4

this is already the default, just FileCheck %s works

raghesh updated this revision to Diff 347285.May 23 2021, 6:36 PM

Addressed the comments. Changed functions to static and made testcase changes

raghesh marked 5 inline comments as done.May 23 2021, 6:37 PM
aeubanks accepted this revision.May 24 2021, 12:01 PM

looks good, thanks!
can you land this yourself or do you need help doing that?

This revision is now accepted and ready to land.May 24 2021, 12:01 PM

looks good, thanks!
can you land this yourself or do you need help doing that?

I do not have commit access. Please push it for me. Thanks a lot for your help and reviews with this.

This revision was automatically updated to reflect the committed changes.