[Polly] In getBuild() use isl::noexception bindings [NFC]

[refactormyself]

Summary

IslAstInfo::getBuild() still uses the ISL's pure C objects as
return values and parameter. The C++ bindings in isl_noexception
(isl::ast_build and isl::ast_node) provides better pointer management

 - Replace the pure C objects with their C++ bindings
 - Refactor calls to this function within IslNodeBuilder.cpp

No functional changes were made.

[patacca]

Always use isl::manage() and isl::manage_copy() when constructing a C++ isl object from a raw C pointer

polly/lib/CodeGen/IslAst.cpp

660

You should use isl::manage() to initialize a isl::ast_build object. The constructor you are using is a private method

[refactormyself]

• use isl::manage() to initialize a isl::ast_build object
• use isl::ast_build() since it's equivalent to isl::ast_build(nullptr)
• use auto

[patacca]

This is the declaration of the function you should use (you can find it in isl-noexceptions.h):
ast_build manage(__isl_take isl_ast_build *ptr);

When creating a isl C++ object from a raw C pointer you should never call the constructor because it's declared as private. You should instead call isl::manage or isl::manage_copy like this:

ast_build BuildCopied = isl::manage_copy(raw_c_pointer);
ast_build Build = isl::manage(raw_c_pointer);

Also you should probably fix the code styling with ninja polly-update-format or ninja polly-check-format

polly/lib/CodeGen/IslAst.cpp

659

That's not what I meant. You should use isl::manage without calling directly the constructor.

[refactormyself]

• call isl::manage() directly instead of the copy constructor- isl::ast_build()
• explicitly declare auto * variables

[Meinersbur]

Thank you for the patch and thanks @patacca for helping with the review.

Could you add NFC to the patch title. It signifies it is a refactoring. (I prefer ir appended as NFC. to the end of the title)

Could you upload the patch with context? See https://www.llvm.org/docs/Phabricator.html#phabricator-request-review-web for details (tldr: Add -U999999 to git diff)

The LLVM coding prefers not using auto except in specific cases. The current code base may not adhere to this everywhere, but the style could be made more conformant when making changes.

The harbormaster build fails indicate that there is some memory corruption going on. At a first glance I suspect it is the return value of IslAstInfo::getBuild. Line 722 of IslAst.cpp calls isl::manage_copy on it, which in case of __isl_give would usually be memory leak. The __isl_give might not actually be correct, repectively IslAstInfo::getBuild should have made a copy before returning.

[refactormyself]

Thank you @Meinersbur and @patacca for your reviews. I will make amendments.
@Meinersbur please, I will appreciate some clarification here:

In D100380#2690549, @Meinersbur wrote:

The harbormaster build fails indicate that there is some memory corruption going on. At a first glance I suspect it is the return value of IslAstInfo::getBuild. Line 722 of IslAst.cpp calls isl::manage_copy on it, which in case of __isl_give would usually be memory leak. The __isl_give might not actually be correct, repectively IslAstInfo::getBuild should have made a copy before returning.

I am trying to understand what is meant by "The __isl_give might not actually be correct ...".
Here is the line of I think is being referred to:

isl::ast_build Build = IslAstInfo::getBuild(isl::manage_copy(Node));

If I understand the code, isl::manage_copy should raise an exception if Node is not "correct"

Do you mean calling isl::manage_copy and not isl::manage on Payload->Build , so it checks and raises an exception?

[Meinersbur]

In D100380#2691641, @refactormyself wrote:

I am trying to understand what is meant by "The __isl_give might not actually be correct ...".
Here is the line of I think is being referred to:

isl::ast_build Build = IslAstInfo::getBuild(isl::manage_copy(Node));

The __isl_give annotation means that getBuild returns an abject for which the caller must assume the ownership of (ie. has the responsibility to eventually free it). isl::manage_copy(IslAstInfo::getBuild(Node)) Makes a copy of the object returned, so that there are 2 objects. The copied object will eventually be freed by the dtor, but the object returned itself is not.

isl::manage_copy(Node) does NOT make a copy of the payload that is returned by getBuild.

If I understand the code, isl::manage_copy should raise an exception if Node is not "correct"

Exceptions are disabled in LLVM. There are some assertions in debug builds, but they don't catch everything.

Do you mean calling isl::manage_copy and not isl::manage on Payload->Build , so it checks and raises an exception?

Moving the isl::manage_copy from the caller of getBuild into the method itself.

[refactormyself]

• return a copy of the build from within getBuild()
• do not copy the Node passed into getBuild()

[Meinersbur]

polly/lib/CodeGen/IslAst.cpp

711

I think this needs to be isl::manage_copy as the previous annotation for the argument was __isl_keep.

polly/lib/CodeGen/IslNodeBuilder.cpp

856–858

I think this needs to be isl::manage_copy as the previous annotation for the argument was __isl_keep.

polly/include/polly/CodeGen/IslAst.h

public:
 
   /// Get minimal dependence distance or nullptr if not available.
   static isl::pw_aff getMinimalDependenceDistance(const isl::ast_node &Node);
 
   /// Get the nodes broken reductions or a nullptr if not available.
   static MemoryAccessSet *getBrokenReductions(const isl::ast_node &Node);
 
   /// Get the nodes build context or a nullptr if not available.
-  static __isl_give isl_ast_build *getBuild(__isl_keep isl_ast_node *Node);
+  static isl::ast_build getBuild(const isl::ast_node &Node);
 
   ///}
 };
 
 struct IslAstAnalysis : public AnalysisInfoMixin<IslAstAnalysis> {
   static AnalysisKey Key;
 
   using Result = IslAstInfo;

polly/lib/CodeGen/IslAst.cpp

 }
 
 IslAstInfo::MemoryAccessSet *
 IslAstInfo::getBrokenReductions(const isl::ast_node &Node) {
   IslAstUserPayload *Payload = getNodePayload(Node);
   return Payload ? &Payload->BrokenReductions : nullptr;
 }
 
-isl_ast_build *IslAstInfo::getBuild(__isl_keep isl_ast_node *Node) {
-  IslAstUserPayload *Payload = getNodePayload(isl::manage_copy(Node));
-  return Payload ? Payload->Build : nullptr;
+isl::ast_build IslAstInfo::getBuild(const isl::ast_node &Node) {
+  IslAstUserPayload *Payload = getNodePayload(Node);
+  isl::ast_build Build =
+      Payload ? isl::manage_copy(Payload->Build) : isl::ast_build();
+  return Build;
 }
 
 static std::unique_ptr<IslAstInfo> runIslAst(
     Scop &Scop,
     function_ref<const Dependences &(Dependences::AnalysisLevel)> GetDeps) {
   // Skip SCoPs in case they're already handled by PPCGCodeGeneration.
   if (Scop.isToBeSkipped())
     return {};
 
   ScopsProcessed++;
 
   const Dependences &D = GetDeps(Dependences::AL_Statement);
 
   if (D.getSharedIslCtx() != Scop.getSharedIslCtx()) {
     LLVM_DEBUG(
         dbgs() << "Got dependence analysis for different SCoP/isl_ctx\n");
     return {};
   }
 
   std::unique_ptr<IslAstInfo> Ast = std::make_unique<IslAstInfo>(Scop, D);
static __isl_give isl_printer *cbPrintUser(__isl_take isl_printer *P,
   for (MemoryAccess *MemAcc : *AccessStmt) {
     P = isl_printer_start_line(P);
 
     if (MemAcc->isRead())
       P = isl_printer_print_str(P, "/* read  */ &");
     else
       P = isl_printer_print_str(P, "/* write */  ");
 
-    isl::ast_build Build = isl::manage_copy(IslAstInfo::getBuild(Node));
+    isl::ast_build Build = IslAstInfo::getBuild(isl::manage_copy(Node));
     if (MemAcc->isAffine()) {
       isl_pw_multi_aff *PwmaPtr =
           MemAcc->applyScheduleToAccessRelation(Build.get_schedule()).release();
       isl::pw_multi_aff Pwma = isl::manage(PwmaPtr);
       isl::ast_expr AccessExpr = Build.access_from(Pwma);
       P = isl_printer_print_ast_expr(P, AccessExpr.get());
     } else {
       P = isl_printer_print_str(

polly/lib/CodeGen/IslNodeBuilder.cpp

 }
 
 __isl_give isl_id_to_ast_expr *
 IslNodeBuilder::createNewAccesses(ScopStmt *Stmt,
                                   __isl_keep isl_ast_node *Node) {
   isl_id_to_ast_expr *NewAccesses =
       isl_id_to_ast_expr_alloc(Stmt->getParent()->getIslCtx().get(), 0);
 
-  auto *Build = IslAstInfo::getBuild(Node);
-  assert(Build && "Could not obtain isl_ast_build from user node");
-  Stmt->setAstBuild(isl::manage_copy(Build));
+  isl::ast_build Build = IslAstInfo::getBuild(isl::manage_copy(Node));
+  assert(!Build.is_null() && "Could not obtain isl_ast_build from user node");
+  Stmt->setAstBuild(Build);
 
   for (auto *MA : *Stmt) {
     if (!MA->hasNewAccessRelation()) {
       if (PollyGenerateExpressions) {
         if (!MA->isAffine())
           continue;
         if (MA->getLatestScopArrayInfo()->getBasePtrOriginSAI())
           continue;
 
         auto *BasePtr =
             dyn_cast<Instruction>(MA->getLatestScopArrayInfo()->getBasePtr());
         if (BasePtr && Stmt->getParent()->getRegion().contains(BasePtr))
           continue;
       } else {
         continue;
       }
     }
     assert(MA->isAffine() &&
            "Only affine memory accesses can be code generated");
 
-    auto Schedule = isl_ast_build_get_schedule(Build);
+    auto *Schedule = isl_ast_build_get_schedule(Build.get());
 
 #ifndef NDEBUG
     if (MA->isRead()) {
       auto Dom = Stmt->getDomain().release();
       auto SchedDom = isl_set_from_union_set(
           isl_union_map_domain(isl_union_map_copy(Schedule)));
       auto AccDom = isl_map_domain(MA->getAccessRelation().release());
       Dom = isl_set_intersect_params(Dom,
isl::set AccDomain =
         isl::manage(isl_pw_multi_aff_domain(isl_pw_multi_aff_copy(PWAccRel)));
     isl::set Context = S.getContext();
     AccDomain = AccDomain.intersect_params(Context);
     if (AccDomain.is_empty()) {
       isl_pw_multi_aff_free(PWAccRel);
       continue;
     }
 
-    auto AccessExpr = isl_ast_build_access_from_pw_multi_aff(Build, PWAccRel);
+    auto *AccessExpr =
+        isl_ast_build_access_from_pw_multi_aff(Build.get(), PWAccRel);
     NewAccesses =
         isl_id_to_ast_expr_set(NewAccesses, MA->getId().release(), AccessExpr);
   }
 
   return NewAccesses;
 }
 
 void IslNodeBuilder::createSubstitutions(__isl_take isl_ast_expr *Expr,