R3: Addressing review comments

Author: anchuraj

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

@@ -3901,8 +3901,9 @@ class CanonicalLoopInfo {
 };
 
 /// ScanInfo holds the information to assist in lowering of Scan reduction.
-/// Before lowering, body of the for loop specifying scan reduction is expected
-/// to have the following structure
+/// Before lowering, the body of the for loop specifying scan reduction is
+/// expected to have the following structure
+///
 ///     Loop Body Entry
 ///        |
 ///     Code before the scan directive
@@ -3943,30 +3944,38 @@ class CanonicalLoopInfo {
 /// Temporary buffer allocations are done in `ScanLoopInit` block before the
 /// lowering of for-loop. The results are copied back to reduction variable in
 /// `ScanLoopFinish` block.
-
 class ScanInfo {
 public:
   /// Dominates the body of the loop before scan directive
   llvm::BasicBlock *OMPBeforeScanBlock = nullptr;
+
   /// Dominates the body of the loop before scan directive
   llvm::BasicBlock *OMPAfterScanBlock = nullptr;
+
   /// Controls the flow to before or after scan blocks
   llvm::BasicBlock *OMPScanDispatch = nullptr;
+
   /// Exit block of loop body
   llvm::BasicBlock *OMPScanLoopExit = nullptr;
+
   /// Block before loop body where scan initializations are done
   llvm::BasicBlock *OMPScanInit = nullptr;
+
   /// Block after loop body where scan finalizations are done
   llvm::BasicBlock *OMPScanFinish = nullptr;
+
   /// If true, it indicates Input phase is lowered; else it indicates
   /// ScanPhase is lowered
   bool OMPFirstScanLoop = false;
+
   /// Maps the private reduction variable to the pointer of the temporary
   /// buffer
   llvm::SmallDenseMap<llvm::Value *, llvm::Value *> *ScanBuffPtrs;
+
   /// Keeps track of value of iteration variable for input/scan loop to be
   /// used for Scan directive lowering
   llvm::Value *IV;
+
   /// Stores the span of canonical loop being lowered to be used for temporary
   /// buffer allocation or Finalization.
   llvm::Value *Span;

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

@@ -4238,14 +4238,14 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::emitScanReduction(
     Builder.SetInsertPoint(LoopBB);
 
     PHINode *Counter = Builder.CreatePHI(Builder.getInt32Ty(), 2);
-    //// size pow2k = 1;
+    // size pow2k = 1;
     PHINode *Pow2K = Builder.CreatePHI(Builder.getInt32Ty(), 2);
     Counter->addIncoming(llvm::ConstantInt::get(Builder.getInt32Ty(), 0),
                          InputBB);
     Pow2K->addIncoming(llvm::ConstantInt::get(Builder.getInt32Ty(), 1),
                        InputBB);
-    //// for (size i = n - 1; i >= 2 ^ k; --i)
-    ////   tmp[i] op= tmp[i-pow2k];
+    // for (size i = n - 1; i >= 2 ^ k; --i)
+    //   tmp[i] op= tmp[i-pow2k];
     llvm::BasicBlock *InnerLoopBB =
         BasicBlock::Create(CurFn->getContext(), "omp.inner.log.scan.body");
     llvm::BasicBlock *InnerExitBB =
@@ -4254,7 +4254,7 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::emitScanReduction(
     Builder.CreateCondBr(CmpI, InnerLoopBB, InnerExitBB);
     emitBlock(InnerLoopBB, CurFn);
     Builder.SetInsertPoint(InnerLoopBB);
-    auto *IVal = Builder.CreatePHI(Builder.getInt32Ty(), 2);
+    PHINode *IVal = Builder.CreatePHI(Builder.getInt32Ty(), 2);
     IVal->addIncoming(NMin1, LoopBB);
     for (ReductionInfo RedInfo : ReductionInfos) {
       Value *ReductionVal = RedInfo.PrivateVariable;
@@ -4329,9 +4329,9 @@ Error OpenMPIRBuilder::emitScanBasedDirectiveIR(
     //   buffer[i] = red;
     // }
     ScanRedInfo->OMPFirstScanLoop = true;
-    auto Result = InputLoopGen();
-    if (Result)
-      return Result;
+    Error Err = InputLoopGen();
+    if (Err)
+      return Err;
   }
   {
     // Emit loop with scan phase:
@@ -4340,9 +4340,9 @@ Error OpenMPIRBuilder::emitScanBasedDirectiveIR(
     //   <scan phase>;
     // }
     ScanRedInfo->OMPFirstScanLoop = false;
-    auto Result = ScanLoopGen(Builder.saveIP());
-    if (Result)
-      return Result;
+    Error Err = ScanLoopGen(Builder.saveIP());
+    if (Err)
+      return Err;
   }
   return Error::success();
 }
@@ -4499,17 +4499,17 @@ OpenMPIRBuilder::createCanonicalScanLoops(
   };
 
   const auto &&InputLoopGen = [&]() -> Error {
-    auto LoopInfo = createCanonicalLoop(Builder.saveIP(), BodyGen, Start, Stop,
-                                        Step, IsSigned, InclusiveStop,
-                                        ComputeIP, Name, true, ScanRedInfo);
+    Expected<CanonicalLoopInfo *> LoopInfo = createCanonicalLoop(
+        Builder.saveIP(), BodyGen, Start, Stop, Step, IsSigned, InclusiveStop,
+        ComputeIP, Name, true, ScanRedInfo);
     if (!LoopInfo)
       return LoopInfo.takeError();
     Result.push_back(*LoopInfo);
     Builder.restoreIP((*LoopInfo)->getAfterIP());
     return Error::success();
   };
   const auto &&ScanLoopGen = [&](LocationDescription Loc) -> Error {
-    auto LoopInfo =
+    Expected<CanonicalLoopInfo *> LoopInfo =
         createCanonicalLoop(Loc, BodyGen, Start, Stop, Step, IsSigned,
                             InclusiveStop, ComputeIP, Name, true, ScanRedInfo);
     if (!LoopInfo)

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

@@ -1,4 +1,3 @@
-
 //===- llvm/unittest/IR/OpenMPIRBuilderTest.cpp - OpenMPIRBuilder tests ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
@@ -5473,22 +5472,22 @@ TEST_F(OpenMPIRBuilderTest, ScanReduction) {
   unsigned NumLog = 0;
   unsigned NumCeil = 0;
   for (Instruction &I : instructions(F)) {
-    if (isa<CallInst>(I)) {
-      CallInst *Call = dyn_cast<CallInst>(&I);
-      auto Name = Call->getCalledFunction()->getName();
-      if (Name.equals_insensitive("malloc")) {
-        NumMallocs += 1;
-      } else if (Name.equals_insensitive("free")) {
-        NumFrees += 1;
-      } else if (Name.equals_insensitive("__kmpc_masked")) {
-        NumMasked += 1;
-      } else if (Name.equals_insensitive("__kmpc_end_masked")) {
-        NumEndMasked += 1;
-      } else if (Name.equals_insensitive("llvm.log2.f64")) {
-        NumLog += 1;
-      } else if (Name.equals_insensitive("llvm.ceil.f64")) {
-        NumCeil += 1;
-      }
+    if (!isa<CallInst>(I))
+      continue;
+    CallInst *Call = dyn_cast<CallInst>(&I);
+    StringRef Name = Call->getCalledFunction()->getName();
+    if (Name.equals_insensitive("malloc")) {
+      NumMallocs += 1;
+    } else if (Name.equals_insensitive("free")) {
+      NumFrees += 1;
+    } else if (Name.equals_insensitive("__kmpc_masked")) {
+      NumMasked += 1;
+    } else if (Name.equals_insensitive("__kmpc_end_masked")) {
+      NumEndMasked += 1;
+    } else if (Name.equals_insensitive("llvm.log2.f64")) {
+      NumLog += 1;
+    } else if (Name.equals_insensitive("llvm.ceil.f64")) {
+      NumCeil += 1;
     }
   }
   EXPECT_EQ(NumBodiesGenerated, 2U);