Privatize / reduce scope of member variables #56
Conversation
|
Marking this as ready for review. There are a lot more classes to do this work on, but I think it's best to keep it to a reviewable chunk at a time. |
| @@ -389,7 +389,6 @@ class DataDepGraph : public llvm::opt_sched::OptSchedDDGWrapperBase, | |||
| int nodeID, InstCount fileSchedOrder, | |||
| InstCount fileSchedCycle, InstCount fileLB, | |||
| InstCount fileUB, int blkNum); | |||
| void AddNode_(SchedInstruction *instPtr, InstCount instNum); | |||
There was a problem hiding this comment.
Why some of these methods are being removed like AddNode_() and SetNum()?
There was a problem hiding this comment.
They are not used anywhere. I removed them because there's no point in having them if they aren't moved. They also reduce the ability to modify some of the variables (having a getter and a setter is barely any better than an exposed variable).
If we want them in the future, it's in the git history; we can resurrect the code.
|
There shouldn't be any regressions in performance, since it's all inlinable. I'll run a test to verify this. |
|
@vangthao95 I can confirm that there are no regressions in compile-times (times.dat). Thereis a minor change in the number of blocks solved optimally: 7 fewer for fp. I suspect that's in the noise. |
|
Sounds good to me then. |
Quincunx271
force-pushed
the
refactor-privatize
branch
from
a0b51f2 to
e0b3440
Compare
Move member variables referenced only by SchedRegion to be private. Variables which could be local variables have been moved to be local variables. Unused variables were removed. Some accessors were added to preserve read-only access to members.
Move member variables to be private as much as possible. Protected accessors had to be added to allow a read-only interface for subclasses.
Quincunx271
force-pushed
the
refactor-privatize
branch
from
e0b3440 to
6b39747
Compare
This patch fixes a crash in debug mode due to a failed [assert](https://github.com/CSUS-LLVM/OptSched/blob/master/lib/Scheduler/sched_basic_data.cpp#L242) when computing the critical path. When computing the critical path, a dir of DIR_FRWD calculates the critical path starting from the root node. To do this, it uses the current critical path of predecessor nodes but we were using the successors instead which returned an invalid value. This was accidentally swapped in #56. Tested with SPEC CPU2006 and SHOC and this patch fixes the crash in debug mode.
See #47
The goal here is to move member variables from
protectedtoprivate, or even to remove them altogether in favor of local variables. Reducing the scope of the variable makes the code easier to follow, as it's clearer where the variable is changed.This is a draft. There's much more work to do. However, I would like to know your thoughts on what's being done before the whole thing is finished.