[SYCL] Refactor the way commands store requirements (#839)

1. Store requirements in all the commands. This prevents potential use
after free.

2. Unified way to access stored requirement. For most of the functions
that are only one requirement that needs to be stored and accessed.
Being able to get it using just parent type(Command) simplifies code
which sets dependency

3. Align/Fix variables names across commands

Signed-off-by: Vlad Romanov <[email protected]>

Author: romanovvlad

sycl/include/CL/sycl/detail/scheduler/commands.hpp

@@ -48,21 +48,21 @@ struct EnqueueResultT {
   cl_int MErrCode;
 };
 
-
 // DepDesc represents dependency between two commands
 struct DepDesc {
-  DepDesc(Command *DepCommand, Requirement *Req, AllocaCommandBase *AllocaCmd)
-      : MDepCommand(DepCommand), MReq(Req), MAllocaCmd(AllocaCmd) {}
+  DepDesc(Command *DepCommand, const Requirement *Req,
+          AllocaCommandBase *AllocaCmd)
+      : MDepCommand(DepCommand), MDepRequirement(Req), MAllocaCmd(AllocaCmd) {}
 
   friend bool operator<(const DepDesc &Lhs, const DepDesc &Rhs) {
-    return std::tie(Lhs.MReq, Lhs.MDepCommand) <
-           std::tie(Rhs.MReq, Rhs.MDepCommand);
+    return std::tie(Lhs.MDepRequirement, Lhs.MDepCommand) <
+           std::tie(Rhs.MDepRequirement, Rhs.MDepCommand);
   }
 
   // The actual dependency command.
   Command *MDepCommand = nullptr;
   // Requirement for the dependency.
-  Requirement *MReq = nullptr;
+  const Requirement *MDepRequirement = nullptr;
   // Allocation command for the memory object we have requirement for.
   // Used to simplify searching for memory handle.
   AllocaCommandBase *MAllocaCmd = nullptr;
@@ -116,10 +116,15 @@ class Command {
 
   std::shared_ptr<event_impl> getEvent() const { return MEvent; }
 
-  virtual ~Command() = default;
-
   virtual void printDot(std::ostream &Stream) const = 0;
 
+  virtual const Requirement *getRequirement() const {
+    assert(!"Internal Error. The command has no stored requirement");
+    return nullptr;
+  }
+
+  virtual ~Command() = default;
+
 protected:
   EventImplPtr MEvent;
   QueueImplPtr MQueue;
@@ -129,22 +134,23 @@ class Command {
                      RT::PiEvent &Event);
   std::vector<RT::PiEvent> prepareEvents(ContextImplPtr Context);
 
-  bool MUseExclusiveQueue = false;
-
   // Private interface. Derived classes should implement this method.
   virtual cl_int enqueueImp() = 0;
 
-public:
+  bool MUseExclusiveQueue = false;
+
   // The type of the command
   CommandType MType;
   // Indicates whether the command is enqueued or not
   std::atomic<bool> MEnqueued;
+  // Mutex used to protect enqueueing from race conditions
+  std::mutex MEnqueueMtx;
+
+public:
   // Contains list of dependencies(edges)
   std::vector<DepDesc> MDeps;
   // Contains list of commands that depend on the command
   std::vector<Command *> MUsers;
-  // Mutex used to protect enqueueing from race conditions
-  std::mutex MEnqueueMtx;
   // Indicates whether the command can be blocked from enqueueing
   bool MIsBlockable = false;
   // Indicates whether the command is blocked from enqueueing
@@ -155,17 +161,17 @@ class Command {
 // lock in the graph, or to merge several nodes into one.
 class EmptyCommand : public Command {
 public:
-  EmptyCommand(QueueImplPtr Queue, Requirement *Req)
+  EmptyCommand(QueueImplPtr Queue, Requirement Req)
       : Command(CommandType::EMPTY_TASK, std::move(Queue)),
-        MStoredRequirement(*Req) {}
+        MRequirement(std::move(Req)) {}
 
-  Requirement *getStoredRequirement() { return &MStoredRequirement; }
+  void printDot(std::ostream &Stream) const final;
+  const Requirement *getRequirement() const final { return &MRequirement; }
 
 private:
-  cl_int enqueueImp() override { return CL_SUCCESS; }
-  void printDot(std::ostream &Stream) const override;
+  cl_int enqueueImp() final { return CL_SUCCESS; }
 
-  Requirement MStoredRequirement;
+  Requirement MRequirement;
 };
 
 // The command enqueues release instance of memory allocated on Host or
@@ -176,32 +182,34 @@ class ReleaseCommand : public Command {
       : Command(CommandType::RELEASE, std::move(Queue)), MAllocaCmd(AllocaCmd) {
   }
 
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
 
 private:
-  cl_int enqueueImp() override;
+  cl_int enqueueImp() final;
 
+  // Command which allocates memory release command should dealocate
   AllocaCommandBase *MAllocaCmd = nullptr;
 };
 
 class AllocaCommandBase : public Command {
 public:
   AllocaCommandBase(CommandType Type, QueueImplPtr Queue, Requirement Req)
-      : Command(Type, Queue), MReleaseCmd(Queue, this), MReq(std::move(Req)) {
-    MReq.MAccessMode = access::mode::read_write;
+      : Command(Type, Queue), MReleaseCmd(Queue, this),
+        MRequirement(std::move(Req)) {
+    MRequirement.MAccessMode = access::mode::read_write;
   }
 
   ReleaseCommand *getReleaseCmd() { return &MReleaseCmd; }
 
-  SYCLMemObjI *getSYCLMemObj() const { return MReq.MSYCLMemObj; }
+  SYCLMemObjI *getSYCLMemObj() const { return MRequirement.MSYCLMemObj; }
 
   void *getMemAllocation() const { return MMemAllocation; }
 
-  Requirement *getAllocationReq() { return &MReq; }
+  const Requirement *getRequirement() const final { return &MRequirement; }
 
 protected:
   ReleaseCommand MReleaseCmd;
-  Requirement MReq;
+  Requirement MRequirement;
   void *MMemAllocation = nullptr;
 };
 
@@ -211,16 +219,19 @@ class AllocaCommand : public AllocaCommandBase {
 public:
   AllocaCommand(QueueImplPtr Queue, Requirement Req,
                 bool InitFromUserData = true)
-      : AllocaCommandBase(CommandType::ALLOCA, std::move(Queue), Req),
+      : AllocaCommandBase(CommandType::ALLOCA, std::move(Queue),
+                          std::move(Req)),
         MInitFromUserData(InitFromUserData) {
-    addDep(DepDesc(nullptr, &MReq, this));
+    addDep(DepDesc(nullptr, getRequirement(), this));
   }
 
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
 
 private:
-  cl_int enqueueImp() override final;
+  cl_int enqueueImp() final;
 
+  // The flag indicates that alloca should try to reuse pointer provided by the
+  // user during memory object construction
   bool MInitFromUserData = false;
 };
 
@@ -231,90 +242,95 @@ class AllocaSubBufCommand : public AllocaCommandBase {
       : AllocaCommandBase(CommandType::ALLOCA_SUB_BUF, std::move(Queue),
                           std::move(Req)),
         MParentAlloca(ParentAlloca) {
-    addDep(DepDesc(MParentAlloca, &MReq, MParentAlloca));
+    addDep(DepDesc(MParentAlloca, getRequirement(), MParentAlloca));
   }
 
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
   AllocaCommandBase *getParentAlloca() { return MParentAlloca; }
 
 private:
-  cl_int enqueueImp() override final;
+  cl_int enqueueImp() final;
 
   AllocaCommandBase *MParentAlloca;
 };
 
 class MapMemObject : public Command {
 public:
-  MapMemObject(AllocaCommandBase *SrcAlloca, Requirement *Req, void **DstPtr,
+  MapMemObject(AllocaCommandBase *SrcAllocaCmd, Requirement Req, void **DstPtr,
                QueueImplPtr Queue);
 
-  AllocaCommandBase *MSrcAlloca = nullptr;
-  void **MDstPtr = nullptr;
-  Requirement MReq;
-
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
+  const Requirement *getRequirement() const final { return &MSrcReq; }
 
 private:
-  cl_int enqueueImp() override;
+  cl_int enqueueImp() final;
+
+  AllocaCommandBase *MSrcAllocaCmd = nullptr;
+  Requirement MSrcReq;
+  void **MDstPtr = nullptr;
 };
 
 class UnMapMemObject : public Command {
 public:
-  UnMapMemObject(AllocaCommandBase *DstAlloca, Requirement *Req, void **SrcPtr,
-                 QueueImplPtr Queue, bool UseExclusiveQueue = false);
+  UnMapMemObject(AllocaCommandBase *DstAllocaCmd, Requirement Req,
+                 void **SrcPtr, QueueImplPtr Queue,
+                 bool UseExclusiveQueue = false);
 
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
+  const Requirement *getRequirement() const final { return &MDstReq; }
 
 private:
-  cl_int enqueueImp() override;
+  cl_int enqueueImp() final;
 
-  AllocaCommandBase *MDstAlloca = nullptr;
-  Requirement MReq;
+  AllocaCommandBase *MDstAllocaCmd = nullptr;
+  Requirement MDstReq;
   void **MSrcPtr = nullptr;
 };
 
 // The command enqueues memory copy between two instances of memory object.
 class MemCpyCommand : public Command {
 public:
-  MemCpyCommand(Requirement SrcReq, AllocaCommandBase *SrcAlloca,
-                Requirement DstReq, AllocaCommandBase *DstAlloca,
+  MemCpyCommand(Requirement SrcReq, AllocaCommandBase *SrcAllocaCmd,
+                Requirement DstReq, AllocaCommandBase *DstAllocaCmd,
                 QueueImplPtr SrcQueue, QueueImplPtr DstQueue,
                 bool UseExclusiveQueue = false);
 
-  QueueImplPtr MSrcQueue;
-  Requirement MSrcReq;
-  AllocaCommandBase *MSrcAlloca = nullptr;
-  Requirement MDstReq;
-  AllocaCommandBase *MDstAlloca = nullptr;
-  Requirement *MAccToUpdate = nullptr;
-
   void setAccessorToUpdate(Requirement *AccToUpdate) {
     MAccToUpdate = AccToUpdate;
   }
 
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
+  const Requirement *getRequirement() const final { return &MDstReq; }
 
 private:
-  cl_int enqueueImp() override;
+  cl_int enqueueImp() final;
+
+  QueueImplPtr MSrcQueue;
+  Requirement MSrcReq;
+  AllocaCommandBase *MSrcAllocaCmd = nullptr;
+  Requirement MDstReq;
+  AllocaCommandBase *MDstAllocaCmd = nullptr;
+  Requirement *MAccToUpdate = nullptr;
 };
 
 // The command enqueues memory copy between two instances of memory object.
 class MemCpyCommandHost : public Command {
 public:
-  MemCpyCommandHost(Requirement SrcReq, AllocaCommandBase *SrcAlloca,
+  MemCpyCommandHost(Requirement SrcReq, AllocaCommandBase *SrcAllocaCmd,
                     Requirement DstReq, void **DstPtr, QueueImplPtr SrcQueue,
                     QueueImplPtr DstQueue);
 
+  void printDot(std::ostream &Stream) const final;
+  const Requirement *getRequirement() const final { return &MDstReq; }
+
+private:
+  cl_int enqueueImp() final;
+
   QueueImplPtr MSrcQueue;
   Requirement MSrcReq;
-  AllocaCommandBase *MSrcAlloca = nullptr;
+  AllocaCommandBase *MSrcAllocaCmd = nullptr;
   Requirement MDstReq;
   void **MDstPtr = nullptr;
-
-  void printDot(std::ostream &Stream) const override;
-
-private:
-  cl_int enqueueImp() override;
 };
 
 // The command enqueues execution of kernel or explicit memory operation.
@@ -326,11 +342,10 @@ class ExecCGCommand : public Command {
 
   void flushStreams();
 
-  void printDot(std::ostream &Stream) const override;
+  void printDot(std::ostream &Stream) const final;
 
 private:
-  // Implementation of enqueueing of ExecCGCommand.
-  cl_int enqueueImp() override;
+  cl_int enqueueImp() final;
 
   AllocaCommandBase *getAllocaForReq(Requirement *Req);
 
@@ -339,20 +354,20 @@ class ExecCGCommand : public Command {
 
 class UpdateHostRequirementCommand : public Command {
 public:
-  UpdateHostRequirementCommand(QueueImplPtr Queue, AllocaCommandBase *AllocaCmd,
-                               Requirement *Req, void **DstPtr)
+  UpdateHostRequirementCommand(QueueImplPtr Queue, Requirement Req,
+                               AllocaCommandBase *SrcAllocaCmd, void **DstPtr)
       : Command(CommandType::UPDATE_REQUIREMENT, std::move(Queue)),
-        MDstPtr(DstPtr), MAllocaCmd(AllocaCmd), MReq(*Req) {}
+        MSrcAllocaCmd(SrcAllocaCmd), MDstReq(std::move(Req)), MDstPtr(DstPtr) {}
 
-  Requirement *getStoredRequirement() { return &MReq; }
+  void printDot(std::ostream &Stream) const final;
+  const Requirement *getRequirement() const final { return &MDstReq; }
 
 private:
-  cl_int enqueueImp() override;
-  void printDot(std::ostream &Stream) const override;
+  cl_int enqueueImp() final;
 
+  AllocaCommandBase *MSrcAllocaCmd = nullptr;
+  Requirement MDstReq;
   void **MDstPtr = nullptr;
-  AllocaCommandBase *MAllocaCmd = nullptr;
-  Requirement MReq;
 };
 
 } // namespace detail