Lorenzo Casalino via llvm-dev
2019-Oct-04 09:01 UTC
[llvm-dev] [MachineScheduler]: SchedBoundary trivially copiable, but "HazardRec" is raw pointer: a design issue?
Hi to everyone, while working with the machine scheduler for a personal project, I came up with the necessity of inserting a backup boundary in the MachineSchedulerStrategy -- specifically, the PostGenericScheduler -- to hold a copy the scheduler's state, in order to implement a really trivial (and really inefficient) backtracking mechanism. This approach leads to a subtle "segmentation fault", when the pass ends and invokes the deleter. The reason of the fault is due to the custom deleter of SchedBoundary, which explicitly deletes the scoreboard object. Follows that, since both the boundary objects, the original and the copy, hold a pointer to the same object, a double "delete" is performed. I think this is an issue deriving from the design: if SchedBoundary is designed to be copiable, then solutions may be: 1. The pointer should be wrapped around a smart_ptr (a shared_ptr, since we want to hold a copy) 2. Define a custom copy-constructor and assign-operator, such that the ScoreBoard object, and not the pointer, is copied. If SchedBoundary was not designed to be copiable, then default copy-costructor/assign-operator should be marked as "deleted". Let me know what do you think about it, and if there's actually the need to submit a patch. Best regards, Lorenzo Casalino
Andrew Trick via llvm-dev
2019-Oct-05 22:31 UTC
[llvm-dev] [MachineScheduler]: SchedBoundary trivially copiable, but "HazardRec" is raw pointer: a design issue?
> On Oct 4, 2019, at 2:01 AM, Lorenzo Casalino via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi to everyone, > > while working with the machine scheduler for a personal project, I came > up with the necessity of > inserting a backup boundary in the MachineSchedulerStrategy -- specifically, > the PostGenericScheduler -- to hold a copy the scheduler's state, in > order to implement a really > trivial (and really inefficient) backtracking mechanism. > > This approach leads to a subtle "segmentation fault", when the pass ends > and invokes the deleter. > > The reason of the fault is due to the custom deleter of SchedBoundary, > which explicitly deletes > the scoreboard object. Follows that, since both the boundary objects, > the original and the copy, hold > a pointer to the same object, a double "delete" is performed. > > I think this is an issue deriving from the design: if SchedBoundary is > designed to be copiable, then > solutions may be: > > 1. The pointer should be wrapped around a smart_ptr (a shared_ptr, > since we want to hold a copy) > > 2. Define a custom copy-constructor and assign-operator, such that > the ScoreBoard object, and not > the pointer, is copied. > > If SchedBoundary was not designed to be copiable, then default > copy-costructor/assign-operator should > be marked as "deleted". > > > Let me know what do you think about it, and if there's actually the need > to submit a patch. > > > Best regards, > > Lorenzo CasalinoHi Lorenzo, SchedBoundary wasn’t designed to be copied. But it looks like could be made to work if you don’t care too much about the overhead. If you do want to copy the scheduling state, don’t you also need to copy the hazard recognizer object? In that case, HazardRec should probably be a unique_ptr AND SchedBoundary's copy constructor needs to copy the hazard recognizer. I’m a *little* worried about encouraging developers to copy the scheduling state because it seems like an easy way introduce inefficiency. But maybe comments telling devs not to do this without being careful would be sufficient. -Andy
Lorenzo Casalino via llvm-dev
2019-Oct-06 19:50 UTC
[llvm-dev] [MachineScheduler]: SchedBoundary trivially copiable, but "HazardRec" is raw pointer: a design issue?
> If you do want to copy the scheduling state, don’t you also need to copy the hazard recognizer object? In that case, HazardRec should probably be a unique_ptr AND SchedBoundary's copy constructor needs to copy the hazard recognizer.You are definitely right.> > I’m a *little* worried about encouraging developers to copy the scheduling state because it seems like an easy way introduce inefficiency. But maybe comments telling devs not to do this without being careful would be sufficient. > > -AndyI share the same concern. Indeed, I'd prefer to prevent the trivial copy, and insert a more efficient mechanism. Actually, for my personal project, I'm working on a small design to make copy-state efficient. May it be of any interest? Meanwhile, I'd issue a patch with the HazardRec object wrapper in a unique_ptr and SchedBoundary with non-default copy-constructor. Thank you, Andrew -- Lorenzo
Possibly Parallel Threads
- Question about LLVM Building Error with "-DLLVM_ENABLE_DUMP" and "RelWithDebInfo"
- [LLVMdev] Questions about MachineScheduler
- Question about LLVM Building Error with "-DLLVM_ENABLE_DUMP" and "RelWithDebInfo"
- [LLVMdev] Questions about MachineScheduler
- Question about LLVM Building Error with "-DLLVM_ENABLE_DUMP" and "RelWithDebInfo"