[Polly] Use separate DT/LI/SE for outlined subfn. NFC. #102460
Conversation
|
|
||
| public: | ||
| /// Change the function that code is emitted into. | ||
| void switchGeneratedFunc(Function *GenFn, DominatorTree *GenDT, |
There was a problem hiding this comment.
GenFn unused? Also in IslExprBuilder.h
There was a problem hiding this comment.
This was intentional since the function is called switchGeneratedFunc. I had stored it in a private field of the class to use it in assertions. Unfortunately, Clang emits a warning for unused fields in non-assert builds and we have some bots with -Werror enabled, so I had to remove the asserts.
I added some assertions to the function itself that use the GenFn.
|
|
||
| // FIXME: This emits a SCEV for GenSE (since GenLRepl will refer to the | ||
| // induction variable of a generated loop), so we should not use SCEVVisitor | ||
| // with it. Howver, it still contains references to the SCoP region. |
There was a problem hiding this comment.
References, where? NewOps shouldn't contain references to the scop, and GenLRepl shouldn't contain references to the scop, so how does Evaluated contain references to the scop? There aren't any other arguments to evaluateAtIteration.
There was a problem hiding this comment.
GenLRepl contains them. The LoopMap/LTS is constructed somewhere without considering uses in a subfunction. If removed, it fails even in some test cases that do not do outlining, some GlobalMap/BBMap still needs to be applied. Probably because some function adds a map entry for value from GenFn instead of updating an existing map entry. See comments for all the exceptions in BlockGenerator::getNewValue. I tried to clean that up before once.
It does not make a difference in practice since SCEVs don't remember which SE they belong to. visitUnknown has another FIXME for this problem. I don't want to spend too much time hunting behind all the FIXMEs since I think @aengelke wants to land #101198.
There was a problem hiding this comment.
Creating derived SCEVs based on a base SCEV from another ScalarEvolution could theoretically cause issues: some places in ScalarEvolution use pointer equality to determine structural equality. But it it doesn't seem to be causing issues, it's probably okay.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
The patch llvm#102460 already implements separate DT/LI/SE for parallel sub function. Crashes have been reported while region generator tries using oringinal function's DT while creating new parallel sub function due to checks in llvm#101198. This patch aims at fixing those cases by switching the DT/LI while generating parallel function using Region Generator. Fixes llvm#117877
The patch #102460 already implements separate DT/LI/SE for parallel sub function. Crashes have been reported while region generator tries using oringinal function's DT while creating new parallel sub function due to checks in #101198. This patch aims at fixing those cases by switching the DT/LI while generating parallel function using Region Generator. Fixes #117877
DominatorTree, LoopInfo, and ScalarEvolution are function-level analyses that expect to be called only on instructions and basic blocks of the function they were original created for. When Polly outlined a parallel loop body into a separate function, it reused the same analyses seemed to work until new checks to be added in #101198.
This patch creates new analyses for the subfunctions. GenDT, GenLI, and GenSE now refer to the analyses of the current region of code. Outside of an outlined function, they refer to the same analysis as used for the SCoP, but are substituted within an outlined function.
Additionally to the cross-function queries of DT/LI/SE, we must not create SCEVs that refer to a mix of expressions for old and generated values. Currently, SCEVs themselves do not "remember" which ScalarEvolution analysis they were created for, but mixing them is just as unexpected as using DT/LI across function boundaries. Some calculations may depend on pointer comparisons, expecting equal SCEV objects to be equal due to SE's uniqueing of SCEV objects, while with a second SE there is a second set of uniqued SCEVs. Hence
SCEVLoopAddRecRewriterwas combined intoScopExpander.SCEVLoopAddRecRewriteronly replaced induction variables but left SCEVUnknowns to reference the old function.SCEVParameterRewriterwould have done so but its job was effectively superseded byScopExpander, and now alsoSCEVLoopAddRecRewriter. Some issues persist put marked with a FIXME in the code. Changing them would possibly cause this patch to be not NFC anymore.This patch was also tested with #101198 applied. That is, Polly's regression tests only, more issues may be discovered on other codes.