[Polly][acc] Do not statically dispatch into IslNodeBuilder's createFor

[philip.pfaffe]

Summary

When GPUNodeBuilder creates loops inside the kernel, it dispatches to
IslNodeBuilder. This however is surprisingly dangerous, since it accesses the
AST Node's user through the wrong type. This patch fixes this problem by
overriding createFor correctly.

This fixes PR35010.

[Meinersbur]

LGTM

include/polly/CodeGen/IslNodeBuilder.h

354 ↗

(On Diff #120557)

Is there some change of meaning to the second argument, or why renaming it?

[philip.pfaffe]

include/polly/CodeGen/IslNodeBuilder.h

354 ↗

(On Diff #120557)

Yes, the meaning changed. Before, KnownParallel was used as a hint to determine whether the created loop should be marked parallel by the Annotator. This analysis now happens in IslNodeBuilder::createFor and is then passed into this method here.

[philip.pfaffe]

This patch is missing a testcase, because I don't know how to properly test this.

Okay to commit without?

[Meinersbur]

For me this is OK. We cannot reliably test undefined behavior. Tools like TypeSanitizer exist for this purpose.

polly/trunk/include/polly/CodeGen/IslNodeBuilder.h

protected:
   /// This function will preload the representing load from @p IAClass and
   /// map all members of @p IAClass to that preloaded value, potentially casted
   /// to the required type.
   ///
   /// @returns False, iff a problem occurred and the load was not preloaded.
   bool preloadInvariantEquivClass(InvariantEquivClassTy &IAClass);
 
   void createForVector(__isl_take isl_ast_node *For, int VectorWidth);
-  void createForSequential(__isl_take isl_ast_node *For, bool KnownParallel);
+  void createForSequential(__isl_take isl_ast_node *For, bool MarkParallel);
 
   /// Create LLVM-IR that executes a for node thread parallel.
   ///
   /// @param For The FOR isl_ast_node for which code is generated.
   void createForParallel(__isl_take isl_ast_node *For);
 
   /// Create new access functions for modified memory accesses.
   ///

polly/trunk/lib/CodeGen/IslNodeBuilder.cpp

if (isl_ast_node_get_type(Body.keep()) != isl_ast_node_mark)
     return false;
   auto Id = Body.mark_get_id();
   if (strcmp(Id.get_name().c_str(), "Loop Vectorizer Disabled") == 0)
     return true;
   return false;
 }
 
 void IslNodeBuilder::createForSequential(__isl_take isl_ast_node *For,
-                                         bool KnownParallel) {
+                                         bool MarkParallel) {
   isl_ast_node *Body;
   isl_ast_expr *Init, *Inc, *Iterator, *UB;
   isl_id *IteratorID;
   Value *ValueLB, *ValueUB, *ValueInc;
   Type *MaxType;
   BasicBlock *ExitBlock;
   Value *IV;
   CmpInst::Predicate Predicate;
-  bool Parallel;
-
-  Parallel = KnownParallel || (IslAstInfo::isParallel(For) &&
-                               !IslAstInfo::isReductionParallel(For));
 
   bool LoopVectorizerDisabled =
       IsLoopVectorizerDisabled(isl::manage(isl_ast_node_copy(For)));
 
   Body = isl_ast_node_for_get_body(For);
 
   // isl_ast_node_for_is_degenerate(For)
   //
void IslNodeBuilder::createForSequential(__isl_take isl_ast_node *For,
   if (MaxType != ValueInc->getType())
     ValueInc = Builder.CreateSExt(ValueInc, MaxType);
 
   // If we can show that LB <Predicate> UB holds at least once, we can
   // omit the GuardBB in front of the loop.
   bool UseGuardBB =
       !SE.isKnownPredicate(Predicate, SE.getSCEV(ValueLB), SE.getSCEV(ValueUB));
   IV = createLoop(ValueLB, ValueUB, ValueInc, Builder, LI, DT, ExitBlock,
-                  Predicate, &Annotator, Parallel, UseGuardBB,
+                  Predicate, &Annotator, MarkParallel, UseGuardBB,
                   LoopVectorizerDisabled);
   IDToValue[IteratorID] = IV;
 
   create(Body);
 
-  Annotator.popLoop(Parallel);
+  Annotator.popLoop(MarkParallel);
 
   IDToValue.erase(IDToValue.find(IteratorID));
 
   Builder.SetInsertPoint(&ExitBlock->front());
 
   isl_ast_node_free(For);
   isl_ast_expr_free(Iterator);
   isl_id_free(IteratorID);
if (1 < VectorWidth && VectorWidth <= 16 && !hasPartialAccesses(For)) {
       return;
     }
   }
 
   if (IslAstInfo::isExecutedInParallel(For)) {
     createForParallel(For);
     return;
   }
-  createForSequential(For, false);
+  bool Parallel =
+      (IslAstInfo::isParallel(For) && !IslAstInfo::isReductionParallel(For));
+  createForSequential(For, Parallel);
 }
 
 void IslNodeBuilder::createIf(__isl_take isl_ast_node *If) {
   isl_ast_expr *Cond = isl_ast_node_if_get_cond(If);
 
   Function *F = Builder.GetInsertBlock()->getParent();
   LLVMContext &Context = F->getContext();
 

polly/trunk/lib/CodeGen/PPCGCodeGeneration.cpp

private:
   ///   - Kernel:        A GPU kernel call (TODO)
   ///   - Data-Transfer: A GPU <-> CPU data-transfer
   ///   - In-kernel synchronization
   ///   - In-kernel memory copy statement
   ///
   /// @param UserStmt The ast node to generate code for.
   virtual void createUser(__isl_take isl_ast_node *UserStmt);
 
+  virtual void createFor(__isl_take isl_ast_node *Node);
+
   enum DataDirection { HOST_TO_DEVICE, DEVICE_TO_HOST };
 
   /// Create code for a data transfer statement
   ///
   /// @param TransferStmt The data transfer statement.
   /// @param Direction The direction in which to transfer data.
   void createDataTransfer(__isl_take isl_ast_node *TransferStmt,
                           enum DataDirection Direction);
case ppcg_kernel_sync:
     isl_ast_node_free(UserStmt);
     return;
   }
 
   isl_ast_expr_free(Expr);
   isl_ast_node_free(UserStmt);
   return;
 }
+
+void GPUNodeBuilder::createFor(__isl_take isl_ast_node *Node) {
+  createForSequential(Node, false);
+}
+
 void GPUNodeBuilder::createKernelCopy(ppcg_kernel_stmt *KernelStmt) {
   isl_ast_expr *LocalIndex = isl_ast_expr_copy(KernelStmt->u.c.local_index);
   LocalIndex = isl_ast_expr_address_of(LocalIndex);
   Value *LocalAddr = ExprBuilder.create(LocalIndex);
   isl_ast_expr *Index = isl_ast_expr_copy(KernelStmt->u.c.index);
   Index = isl_ast_expr_address_of(Index);
   Value *GlobalAddr = ExprBuilder.create(Index);