Page MenuHomePhabricator

[Polly] Codegen for simple full function Scops
Needs ReviewPublic

Authored by manasij7479 on Aug 20 2017, 1:22 PM.

Details

Reviewers
grosser
bollu
Summary

Code generation for full functions on -polly-detect-full-functions.
Not yet tested with functions having multiple returns or unreachables.

Diff Detail

Repository
rL LLVM

Event Timeline

manasij7479 created this revision.Aug 20 2017, 1:22 PM
manasij7479 added inline comments.Aug 20 2017, 1:25 PM
lib/CodeGen/BlockGenerators.cpp
890

Not sure what should be done for these.

lib/CodeGen/CodeGeneration.cpp
156

Might be a better idea to reuse the UnifyFunctionExitNodes pass.

manasij7479 added inline comments.Aug 20 2017, 1:30 PM
lib/CodeGen/CodeGeneration.cpp
273

Probably not a good idea. OTOH, I can not figure out how to update the original DT properly.

Nice work!

lib/CodeGen/BlockGenerators.cpp
890

There are no escaping uses if the SCoP covers the whole function, so these should not be necessary.

However, function may return (ret double %val) values. ScopDetection allows them for TopLevelRegions and must be handled somehow. UnifyFunctionExitNodes might handle this already, but we should have a test case.

lib/CodeGen/CodeGeneration.cpp
156

[Style] According to LLVM Coding Standards, functions start with a lower case letter.

Can this function be made static?

A doxygen description for what the function does would be nice.

170–182

Why is it necessary to have only a single unreachable terminator? AFAIK unreachable's are modelled as error blocks, i.e. are assumed to be not executed at all, so are never code-generated.

188

[Suggestion] A function without a return would be strange since we do not model infinite loops in SCoPs. We therefore could place an llvm_unreachable here.

197

[Style] Empty comment line.

199

[Suggestion] Usually block names for code generated by Polly has a polly. prefix to make it better distinguishable,

236

[Suggestion] It would be really nice if we could handle in generally not just for TopLevelRegions. It would allow us to get rid of the CodePreparationPass pass.

344

[Serious] Please avoid duplicating code for special cases. It increases the burden of maintenance in that we have to fix the same bugs in multiple code paths.

lib/CodeGen/Utils.cpp
24–32

[Style] When export a function in the header, please move the documentation to the declaration in the header.

test/Isl/CodeGen/full-function.ll
3–5

For a minimal explementary function we maybe should verify the entire function.

bollu edited edge metadata.Aug 20 2017, 4:26 PM

Thanks a lot for the patch! I have some stylistic comments to make. However, I do not understand the related codebase well enough, so I'll leave that for the others :).

lib/CodeGen/CodeGeneration.cpp
160

consider SmallVector? It should be more efficient than std::vector with a decent choice of a default, say, 4

206

Personal opinion: Use the IRBuilder for building this form of IR? I find it much cleaner to read IR that's built using IRBuilder.

221

Personal opinion: I would prefer extracting out the ReturnInst from the BB and then use eraseFromParent on that instruction.

for (BasicBlock *BB : ReturningBlocks) {
    ReturnInst *RI = cast<ReturnInst>(BB->getTerminatorInst());
    ...
    RI->eraseFromParent();
}
241

asserts usually have a message in polly.

285

From what I can tell, there is code duplication between this and IslNodeBuilder. Is there a way to unify these two branches of code?

manasij7479 marked 8 inline comments as done.

Address some of the issues.

manasij7479 marked 2 inline comments as done.Aug 21 2017, 10:39 AM
manasij7479 added inline comments.
lib/CodeGen/CodeGeneration.cpp
344

This needs some more discussion.

manasij7479 marked 3 inline comments as done.

Refactor to remove code duplication

Test CFG structure