[PATCH] D40803: Port SCEVAffinator to the isl c++ bindings

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

After removing the isl memory annotations: LGTM.



================
Comment at: include/polly/Support/SCEVAffinator.h:50
   /// @returns The isl representation of the SCEV @p E in @p Domain.
   __isl_give PWACtx getPwAff(const llvm::SCEV *E,
                              llvm::BasicBlock *BB = nullptr);
----------------
Please remove isl memory annotations.


================
Comment at: include/polly/Support/SCEVAffinator.h:86
   /// Return a PWACtx for @p PWA that is always valid.
-  __isl_give PWACtx getPWACtxFromPWA(__isl_take isl_pw_aff *PWA);
+  __isl_give PWACtx getPWACtxFromPWA(__isl_take isl::pw_aff PWA);
 
----------------
You can remove `__isl_take`.


================
Comment at: include/polly/Support/SCEVAffinator.h:94-95
   /// @returns The expr @p PWA modulo the size constraints of @p ExprType.
-  __isl_give isl_pw_aff *addModuloSemantic(__isl_take isl_pw_aff *PWA,
+  __isl_give isl::pw_aff addModuloSemantic(__isl_take isl::pw_aff PWA,
                                            llvm::Type *ExprType) const;
 
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:53-62
-/// Helper to free a PWACtx object.
-static void freePWACtx(__isl_take PWACtx &PWAC) {
-  isl_pw_aff_free(PWAC.first);
-  isl_set_free(PWAC.second);
-}
-
-/// Helper to copy a PWACtx object.
----------------
nice


================
Comment at: lib/Support/SCEVAffinator.cpp:66-67
-///
-/// Note that @p PWAC will be "free" (deallocated) if this function returns
-/// true, but not if this function returns false.
-static bool isTooComplex(PWACtx &PWAC) {
----------------
Thanks making the API simpler.


================
Comment at: lib/Support/SCEVAffinator.cpp:115
 
 __isl_give PWACtx SCEVAffinator::getPwAff(const SCEV *Expr, BasicBlock *BB) {
   this->BB = BB;
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:242
 
 __isl_give PWACtx SCEVAffinator::visitConstant(const SCEVConstant *Expr) {
   ConstantInt *Value = Expr->getValue();
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:264
 
 __isl_give PWACtx
 SCEVAffinator::visitTruncateExpr(const SCEVTruncateExpr *Expr) {
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:379
 
 __isl_give PWACtx SCEVAffinator::visitMulExpr(const SCEVMulExpr *Expr) {
   PWACtx Prod = visit(Expr->getOperand(0));
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:442
 
 __isl_give PWACtx SCEVAffinator::visitUMaxExpr(const SCEVUMaxExpr *Expr) {
   llvm_unreachable("SCEVUMaxExpr not yet supported");
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:446
 
 __isl_give PWACtx SCEVAffinator::visitUDivExpr(const SCEVUDivExpr *Expr) {
   // The handling of unsigned division is basically the same as for signed
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:484
 
 __isl_give PWACtx SCEVAffinator::visitSDivInstruction(Instruction *SDiv) {
   assert(SDiv->getOpcode() == Instruction::SDiv && "Assumed SDiv instruction!");
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:501
 
 __isl_give PWACtx SCEVAffinator::visitSRemInstruction(Instruction *SRem) {
   assert(SRem->getOpcode() == Instruction::SRem && "Assumed SRem instruction!");
----------------
Please remove isl memory annotations.


================
Comment at: lib/Support/SCEVAffinator.cpp:518
 
 __isl_give PWACtx SCEVAffinator::visitUnknown(const SCEVUnknown *Expr) {
   if (Instruction *I = dyn_cast<Instruction>(Expr->getValue())) {
----------------
Please remove isl memory annotations.


Repository:
  rPLO Polly

https://reviews.llvm.org/D40803