Decoupling the unrolling logic into three different functions. The shouldPragmaUnroll() covers the 1st and 2nd priorities of the previous code, the shouldFullUnroll() covers the 3rd, and the shouldPartialUnroll() covers the 5th. The output of each function, Optional<unsigned>, could be a value for UP.Count, which means unrolling factor has been set, or None, which means decision hasn't been made yet and should try the next priority.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for fixing the commits! Please don't forget to add a commit message, both in git (i.e. git --amend and add a message), and then copy it in the summary here.
Like discussed offline, I think we should explore next splitting the logic of shouldUnroll into its constituent sub-pieces, i.e. shouldFullyUnroll, shouldPartiallyUnroll, etc; that will simplify their interface - the output of one of these can be an Optional<unsigned>, where the unsigned is the decided trip count (and None means 'no' such unrolling)
But maybe we can do that in a next patch - @jdoerfert , WDYT?
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp | ||
---|---|---|
1075 | remove the commented line altogether, also the free space. |
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp | ||
---|---|---|
895 | Use a struct with a name, not a pair of bools. It is much harder to determine what the bools mean this way, if you return an object of type struct SomeGoodName { bool InfoA; bool InfoB; } it is clear what the bools mean. | |
1012 | We have duplication here. Can you query this once and put everything into a "info struct" and then pass that to all the functions here. Make the functions static if you can. |
[WIP][NFC] Changing bool, as the return arg of unrolling logics, to optional<unsigned>
Some nits, but lgtm assuming nits are fixed, tests pass, and also the message of this review is updated (e.g. remove WIP, and add a description for the change)
Wait for @jdoerfert if he has more feedback, too.
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp | ||
---|---|---|
810 | return None; (same below) | |
934 | Up.Count = *UnrollFactor. The check above ensures the dereference succeeds. | |
985 | same as above - *UnrollFactor | |
1017 | same as above - *UnrollFactor |
[NFC] Decoupling the unrolling logic into three different functions. The shouldPragmaUnroll() covers the 1st and 2nd priorities of the previous code, the shouldFullUnroll() covers the 3rd, and the shouldPartialUnroll() covers the 5th. The output of each function, Optional<unsigned>, could be a value for UP.Count, which means unrolling factor has been set, or None, which means decision hasn't been made yet and should try the next priority.
[NFC] Decoupling the unrolling logic
Decoupling the unrolling logic into three different functions. The shouldPragmaUnroll() covers the 1st and 2nd priorities of the previous code, the shouldFullUnroll() covers the 3rd, and the shouldPartialUnroll() covers the 5th. The output of each function, Optional<unsigned>, could be a value for UP.Count, which means unrolling factor has been set, or None, which means decision hasn't been made yet and should try the next priority.
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp | ||
---|---|---|
323 | Nit: bool and unsigned don't get initialized, so to avoid nasty nondeterministic bugs, easiest to assign them on declaration, here, i.e. bool UserUnrollCount = false; unsigned PragmaCount = 0; etc. This improves also maintainability, because it's *much* easier for folks working with the code to follow the same pattern if they add more fields, and also figure out where the object is initialized (less reading required, basically) | |
766 | do static_cast<uint64_t>(same stuff) same below | |
894 | delete this line, it's spurious | |
903 | One thing to consider: if PragmaInfo are just carriers of those fields, and (currently) they are meant to be set once, I'd make the code even more maintainable if:
that way, it's very clear that a PragmaInfo object has intentionally initialized fields, and also that they can't ever be changed. | |
937 | why binary or? i.e. did you mean || ? | |
964 | remove spurious line | |
999 | remove spurious line | |
1037–1038 | line | |
1040 | line |
By including <optional> this makes LLVM require C++17, which it shouldn't: https://llvm.org/docs/CodingStandards.html#c-standard-versions
Nit: bool and unsigned don't get initialized, so to avoid nasty nondeterministic bugs, easiest to assign them on declaration, here, i.e. bool UserUnrollCount = false; unsigned PragmaCount = 0; etc. This improves also maintainability, because it's *much* easier for folks working with the code to follow the same pattern if they add more fields, and also figure out where the object is initialized (less reading required, basically)