Merged main:8fb28e45ce04 into amd-gfx:345ce9d5346e

Local branch amd-gfx 345ce9d Merged main:7fcbb64fca5e into amd-gfx:b56d6b265611
Remote branch main 8fb28e4 [BOLT] Fix data race in MCPlusBuilder::getOrCreateAnnotationIndex (llvm#67004)

Author: SC llvm team

bolt/include/bolt/Core/MCPlusBuilder.h

@@ -29,6 +29,7 @@
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/RWMutex.h"
 #include <cassert>
 #include <cstdint>
 #include <map>
@@ -166,6 +167,10 @@ class MCPlusBuilder {
   /// Names of non-standard annotations.
   SmallVector<std::string, 8> AnnotationNames;
 
+  /// A mutex that is used to control parallel accesses to
+  /// AnnotationNameIndexMap and AnnotationsNames.
+  mutable llvm::sys::RWMutex AnnotationNameMutex;
+
   /// Allocate the TailCall annotation value. Clients of the target-specific
   /// MCPlusBuilder classes must use convert/lower/create* interfaces instead.
   void setTailCall(MCInst &Inst);
@@ -1775,6 +1780,7 @@ class MCPlusBuilder {
 
   /// Return annotation index matching the \p Name.
   std::optional<unsigned> getAnnotationIndex(StringRef Name) const {
+    std::shared_lock<llvm::sys::RWMutex> Lock(AnnotationNameMutex);
     auto AI = AnnotationNameIndexMap.find(Name);
     if (AI != AnnotationNameIndexMap.end())
       return AI->second;
@@ -1784,10 +1790,10 @@ class MCPlusBuilder {
   /// Return annotation index matching the \p Name. Create a new index if the
   /// \p Name wasn't registered previously.
   unsigned getOrCreateAnnotationIndex(StringRef Name) {
-    auto AI = AnnotationNameIndexMap.find(Name);
-    if (AI != AnnotationNameIndexMap.end())
-      return AI->second;
+    if (std::optional<unsigned> Index = getAnnotationIndex(Name))
+      return *Index;
 
+    std::unique_lock<llvm::sys::RWMutex> Lock(AnnotationNameMutex);
     const unsigned Index =
         AnnotationNameIndexMap.size() + MCPlus::MCAnnotation::kGeneric;
     AnnotationNameIndexMap.insert(std::make_pair(Name, Index));

flang/include/flang/Lower/Allocatable.h

@@ -57,6 +57,11 @@ void genDeallocateBox(AbstractConverter &converter,
                       const fir::MutableBoxValue &box, mlir::Location loc,
                       mlir::Value declaredTypeDesc = {});
 
+/// Deallocate an allocatable if it is allocated at the end of its lifetime.
+void genDeallocateIfAllocated(AbstractConverter &converter,
+                              const fir::MutableBoxValue &box,
+                              mlir::Location loc);
+
 /// Create a MutableBoxValue for an allocatable or pointer entity.
 /// If the variables is a local variable that is not a dummy, it will be
 /// initialized to unallocated/diassociated status.

flang/include/flang/Optimizer/Builder/MutableBox.h

@@ -120,19 +120,22 @@ void finalizeRealloc(fir::FirOpBuilder &builder, mlir::Location loc,
                      bool takeLboundsIfRealloc,
                      const MutableBoxReallocation &realloc);
 
-/// Finalize a mutable box if it is allocated or associated. This includes both
-/// calling the finalizer, if any, and deallocating the storage.
-void genFinalization(fir::FirOpBuilder &builder, mlir::Location loc,
-                     const fir::MutableBoxValue &box);
+/// Deallocate a mutable box with fir.freemem if it is allocated or associated.
+/// This only deallocates the storage and does not call finalization, the
+/// mutable box is not nullified.
+void genFreememIfAllocated(fir::FirOpBuilder &builder, mlir::Location loc,
+                           const fir::MutableBoxValue &box);
 
 void genInlinedAllocation(fir::FirOpBuilder &builder, mlir::Location loc,
                           const fir::MutableBoxValue &box,
                           mlir::ValueRange lbounds, mlir::ValueRange extents,
                           mlir::ValueRange lenParams, llvm::StringRef allocName,
                           bool mustBeHeap = false);
 
-mlir::Value genInlinedDeallocate(fir::FirOpBuilder &builder, mlir::Location loc,
-                                 const fir::MutableBoxValue &box);
+/// Deallocate an mutable box storage with fir.freemem without calling any
+/// final procedures. The mutable box is not nullified.
+mlir::Value genFreemem(fir::FirOpBuilder &builder, mlir::Location loc,
+                       const fir::MutableBoxValue &box);
 
 /// When the MutableBoxValue was passed as a fir.ref<fir.box> to a call that may
 /// have modified it, update the MutableBoxValue according to the

flang/lib/Lower/Allocatable.cpp

@@ -746,7 +746,7 @@ static mlir::Value genDeallocate(fir::FirOpBuilder &builder, mlir::Location loc,
   if (!box.isDerived() && !box.isPolymorphic() &&
       !box.isUnlimitedPolymorphic() && !errorManager.hasStatSpec() &&
       !useAllocateRuntime) {
-    return fir::factory::genInlinedDeallocate(builder, loc, box);
+    return fir::factory::genFreemem(builder, loc, box);
   }
   // Use runtime calls to deallocate descriptor cases. Sync MutableBoxValue
   // with its descriptor before and after calls if needed.
@@ -770,6 +770,26 @@ void Fortran::lower::genDeallocateBox(
   genDeallocate(builder, loc, box, errorManager, declaredTypeDesc);
 }
 
+void Fortran::lower::genDeallocateIfAllocated(
+    Fortran::lower::AbstractConverter &converter,
+    const fir::MutableBoxValue &box, mlir::Location loc) {
+  fir::FirOpBuilder &builder = converter.getFirOpBuilder();
+  mlir::Value isAllocated =
+      fir::factory::genIsAllocatedOrAssociatedTest(builder, loc, box);
+  builder.genIfThen(loc, isAllocated)
+      .genThen([&]() {
+        if (mlir::Type eleType = box.getEleTy();
+            eleType.isa<fir::RecordType>() && box.isPolymorphic()) {
+          mlir::Value declaredTypeDesc = builder.create<fir::TypeDescOp>(
+              loc, mlir::TypeAttr::get(eleType));
+          genDeallocateBox(converter, box, loc, declaredTypeDesc);
+        } else {
+          genDeallocateBox(converter, box, loc);
+        }
+      })
+      .end();
+}
+
 static void preDeallocationAction(Fortran::lower::AbstractConverter &converter,
                                   fir::FirOpBuilder &builder,
                                   mlir::Value beginOpValue,
@@ -813,12 +833,13 @@ void Fortran::lower::genDeallocateStmt(
         genMutableBoxValue(converter, loc, allocateObject);
     mlir::Value declaredTypeDesc = {};
     if (box.isPolymorphic()) {
-      assert(symbol.GetType());
-      if (const Fortran::semantics::DerivedTypeSpec *derivedTypeSpec =
-              symbol.GetType()->AsDerived()) {
-        declaredTypeDesc =
-            Fortran::lower::getTypeDescAddr(converter, loc, *derivedTypeSpec);
-      }
+      mlir::Type eleType = box.getEleTy();
+      if (eleType.isa<fir::RecordType>())
+        if (const Fortran::semantics::DerivedTypeSpec *derivedTypeSpec =
+                symbol.GetType()->AsDerived()) {
+          declaredTypeDesc =
+              Fortran::lower::getTypeDescAddr(converter, loc, *derivedTypeSpec);
+        }
     }
     mlir::Value beginOpValue =
         genDeallocate(builder, loc, box, errorManager, declaredTypeDesc);

flang/lib/Lower/Bridge.cpp

@@ -706,14 +706,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
             return;
           }
           // deallocate allocated in createHostAssociateVarClone value
-          mlir::Value needs_dealloc =
-              fir::factory::genIsAllocatedOrAssociatedTest(*builder, loc,
-                                                           new_box);
-          builder->genIfThen(loc, needs_dealloc)
-              .genThen([&]() {
-                Fortran::lower::genDeallocateBox(*this, new_box, loc);
-              })
-              .end();
+          Fortran::lower::genDeallocateIfAllocated(*this, new_box, loc);
         },
         [&](const auto &) -> void {
           // Do nothing

flang/lib/Lower/ConvertCall.cpp

@@ -461,10 +461,13 @@ fir::ExtendedValue Fortran::lower::genCallOpAndResult(
     allocatedResult->match(
         [&](const fir::MutableBoxValue &box) {
           if (box.isAllocatable() && !cleanupWithDestroy) {
-            // 9.7.3.2 point 4. Finalize allocatables.
+            // 9.7.3.2 point 4. Deallocate allocatable results. Note that
+            // finalization was done independently by calling
+            // genDerivedTypeDestroy above and is not triggered by this inline
+            // deallocation.
             fir::FirOpBuilder *bldr = &converter.getFirOpBuilder();
             stmtCtx.attachCleanup([bldr, loc, box]() {
-              fir::factory::genFinalization(*bldr, loc, box);
+              fir::factory::genFreememIfAllocated(*bldr, loc, box);
             });
           }
         },

flang/lib/Lower/ConvertExpr.cpp

@@ -2646,26 +2646,14 @@ class ScalarExprLowering {
         }
         // Passing a POINTER to a POINTER, or an ALLOCATABLE to an ALLOCATABLE.
         fir::MutableBoxValue mutableBox = genMutableBoxValue(*expr);
+        if (fir::isAllocatableType(argTy) && arg.isIntentOut() &&
+            Fortran::semantics::IsBindCProcedure(*procRef.proc().GetSymbol()))
+          Fortran::lower::genDeallocateIfAllocated(converter, mutableBox, loc);
         mlir::Value irBox =
             fir::factory::getMutableIRBox(builder, loc, mutableBox);
         caller.placeInput(arg, irBox);
         if (arg.mayBeModifiedByCall())
           mutableModifiedByCall.emplace_back(std::move(mutableBox));
-        if (fir::isAllocatableType(argTy) && arg.isIntentOut() &&
-            Fortran::semantics::IsBindCProcedure(*procRef.proc().GetSymbol())) {
-          if (mutableBox.isDerived() || mutableBox.isPolymorphic() ||
-              mutableBox.isUnlimitedPolymorphic()) {
-            mlir::Value isAlloc = fir::factory::genIsAllocatedOrAssociatedTest(
-                builder, loc, mutableBox);
-            builder.genIfThen(loc, isAlloc)
-                .genThen([&]() {
-                  Fortran::lower::genDeallocateBox(converter, mutableBox, loc);
-                })
-                .end();
-          } else {
-            Fortran::lower::genDeallocateBox(converter, mutableBox, loc);
-          }
-        }
         continue;
       }
       if (arg.passBy == PassBy::BaseAddress || arg.passBy == PassBy::BoxChar ||

flang/lib/Lower/ConvertVariable.cpp

@@ -749,40 +749,17 @@ static void deallocateIntentOut(Fortran::lower::AbstractConverter &converter,
       }
       mlir::Location loc = converter.getCurrentLocation();
       fir::FirOpBuilder &builder = converter.getFirOpBuilder();
-      auto genDeallocateWithTypeDesc = [&]() {
-        if (mutBox->isDerived() || mutBox->isPolymorphic() ||
-            mutBox->isUnlimitedPolymorphic()) {
-          mlir::Value isAlloc = fir::factory::genIsAllocatedOrAssociatedTest(
-              builder, loc, *mutBox);
-          builder.genIfThen(loc, isAlloc)
-              .genThen([&]() {
-                if (mutBox->isPolymorphic()) {
-                  mlir::Value declaredTypeDesc;
-                  assert(sym.GetType());
-                  if (const Fortran::semantics::DerivedTypeSpec
-                          *derivedTypeSpec = sym.GetType()->AsDerived()) {
-                    declaredTypeDesc = Fortran::lower::getTypeDescAddr(
-                        converter, loc, *derivedTypeSpec);
-                  }
-                  genDeallocateBox(converter, *mutBox, loc, declaredTypeDesc);
-                } else {
-                  genDeallocateBox(converter, *mutBox, loc);
-                }
-              })
-              .end();
-        } else {
-          genDeallocateBox(converter, *mutBox, loc);
-        }
-      };
 
       if (Fortran::semantics::IsOptional(sym)) {
         auto isPresent = builder.create<fir::IsPresentOp>(
             loc, builder.getI1Type(), fir::getBase(extVal));
         builder.genIfThen(loc, isPresent)
-            .genThen([&]() { genDeallocateWithTypeDesc(); })
+            .genThen([&]() {
+              Fortran::lower::genDeallocateIfAllocated(converter, *mutBox, loc);
+            })
             .end();
       } else {
-        genDeallocateWithTypeDesc();
+        Fortran::lower::genDeallocateIfAllocated(converter, *mutBox, loc);
       }
     }
   }

flang/lib/Optimizer/Builder/MutableBox.cpp

@@ -454,29 +454,27 @@ mlir::Value fir::factory::genIsNotAllocatedOrAssociatedTest(
   return builder.genIsNullAddr(loc, addr);
 }
 
-/// Generate finalizer call and inlined free. This does not check that the
+/// Call freemem. This does not check that the
 /// address was allocated.
-static void genFinalizeAndFree(fir::FirOpBuilder &builder, mlir::Location loc,
-                               mlir::Value addr) {
-  // TODO: call finalizer if any.
-
+static void genFreemem(fir::FirOpBuilder &builder, mlir::Location loc,
+                       mlir::Value addr) {
   // A heap (ALLOCATABLE) object may have been converted to a ptr (POINTER),
   // so make sure the heap type is restored before deallocation.
   auto cast = builder.createConvert(
       loc, fir::HeapType::get(fir::dyn_cast_ptrEleTy(addr.getType())), addr);
   builder.create<fir::FreeMemOp>(loc, cast);
 }
 
-void fir::factory::genFinalization(fir::FirOpBuilder &builder,
-                                   mlir::Location loc,
-                                   const fir::MutableBoxValue &box) {
+void fir::factory::genFreememIfAllocated(fir::FirOpBuilder &builder,
+                                         mlir::Location loc,
+                                         const fir::MutableBoxValue &box) {
   auto addr = MutablePropertyReader(builder, loc, box).readBaseAddress();
   auto isAllocated = builder.genIsNotNullAddr(loc, addr);
   auto ifOp = builder.create<fir::IfOp>(loc, isAllocated,
                                         /*withElseRegion=*/false);
   auto insPt = builder.saveInsertionPoint();
   builder.setInsertionPointToStart(&ifOp.getThenRegion().front());
-  genFinalizeAndFree(builder, loc, addr);
+  ::genFreemem(builder, loc, addr);
   builder.restoreInsertionPoint(insPt);
 }
 
@@ -753,12 +751,11 @@ void fir::factory::genInlinedAllocation(
                 fir::MustBeHeapAttr::get(builder.getContext(), mustBeHeap));
 }
 
-mlir::Value
-fir::factory::genInlinedDeallocate(fir::FirOpBuilder &builder,
-                                   mlir::Location loc,
-                                   const fir::MutableBoxValue &box) {
+mlir::Value fir::factory::genFreemem(fir::FirOpBuilder &builder,
+                                     mlir::Location loc,
+                                     const fir::MutableBoxValue &box) {
   auto addr = MutablePropertyReader(builder, loc, box).readBaseAddress();
-  genFinalizeAndFree(builder, loc, addr);
+  ::genFreemem(builder, loc, addr);
   MutablePropertyWriter{builder, loc, box}.setUnallocatedStatus();
   return addr;
 }
@@ -909,8 +906,7 @@ void fir::factory::finalizeRealloc(fir::FirOpBuilder &builder,
         auto heap = fir::getBase(realloc.newValue);
         auto extents = fir::factory::getExtents(loc, builder, realloc.newValue);
         builder.genIfThen(loc, realloc.oldAddressWasAllocated)
-            .genThen(
-                [&]() { genFinalizeAndFree(builder, loc, realloc.oldAddress); })
+            .genThen([&]() { ::genFreemem(builder, loc, realloc.oldAddress); })
             .end();
         MutablePropertyWriter{builder, loc, box}.updateMutableBox(
             heap, lbs, extents, lengths);

flang/test/Lower/Intrinsics/system_clock.f90

@@ -53,13 +53,13 @@ subroutine ss(count)
   ! CHECK:     fir.store %[[V_29]] to %arg0 : !fir.ref<i64>
   ! CHECK:   }
   ! CHECK:   %[[V_12:[0-9]+]] = fir.convert %[[V_9]] : (!fir.ptr<i64>) -> i64
-  ! CHECK:   %[[V_13:[0-9]+]] = arith.cmpi ne, %[[V_12]], %c0{{.*}}_i64 : i64
+  ! CHECK:   %[[V_13:[0-9]+]] = arith.cmpi ne, %[[V_12]], %c0{{.*}} : i64
   ! CHECK:   fir.if %[[V_13]] {
-  ! CHECK:     %[[V_29]] = fir.call @_FortranASystemClockCountRate(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
+  ! CHECK:     %[[V_29:[0-9]+]] = fir.call @_FortranASystemClockCountRate(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
   ! CHECK:     fir.store %[[V_29]] to %[[V_9]] : !fir.ptr<i64>
   ! CHECK:   }
   ! CHECK:   %[[V_14:[0-9]+]] = fir.convert %[[V_10]] : (!fir.heap<i64>) -> i64
-  ! CHECK:   %[[V_15:[0-9]+]] = arith.cmpi ne, %[[V_14]], %c0{{.*}}_i64_0 : i64
+  ! CHECK:   %[[V_15:[0-9]+]] = arith.cmpi ne, %[[V_14]], %c0{{.*}} : i64
   ! CHECK:   fir.if %[[V_15]] {
   ! CHECK:     %[[V_29]] = fir.call @_FortranASystemClockCountMax(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
   ! CHECK:     fir.store %[[V_29]] to %[[V_10]] : !fir.heap<i64>
@@ -77,24 +77,24 @@ subroutine ss(count)
   ! CHECK:     %[[V_39:[0-9]+]] = fir.call @_FortranAioOutputInteger64(%[[V_31]], %[[V_38]]) {{.*}}: (!fir.ref<i8>, i64) -> i1
   ! CHECK:     %[[V_40:[0-9]+]] = fir.call @_FortranAioEndIoStatement(%[[V_31]]) {{.*}}: (!fir.ref<i8>) -> i32
   ! CHECK:   } else {
-  ! CHECK:     %[[V_29]] = fir.load %[[V_4]] : !fir.ref<!fir.ptr<i64>>
+  ! CHECK:     %[[V_29:[0-9]+]] = fir.load %[[V_4]] : !fir.ref<!fir.ptr<i64>>
   ! CHECK:     %[[V_30:[0-9]+]] = fir.load %[[V_1]] : !fir.ref<!fir.heap<i64>>
-  ! CHECK:     %[[V_31]] = fir.convert %[[V_29]] : (!fir.ptr<i64>) -> i64
-  ! CHECK:     %[[V_32]] = arith.cmpi ne, %[[V_31]], %c0{{.*}}_i64_3 : i64
+  ! CHECK:     %[[V_31:[0-9]+]] = fir.convert %[[V_29]] : (!fir.ptr<i64>) -> i64
+  ! CHECK:     %[[V_32:[0-9]+]] = arith.cmpi ne, %[[V_31]], %c0{{.*}} : i64
   ! CHECK:     fir.if %[[V_32]] {
   ! CHECK:       %[[V_45:[0-9]+]] = fir.call @_FortranASystemClockCountRate(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
   ! CHECK:       fir.store %[[V_45]] to %[[V_29]] : !fir.ptr<i64>
   ! CHECK:     }
-  ! CHECK:     %[[V_33]] = fir.convert %[[V_30]] : (!fir.heap<i64>) -> i64
-  ! CHECK:     %[[V_34]] = arith.cmpi ne, %[[V_33]], %c0{{.*}}_i64_4 : i64
+  ! CHECK:     %[[V_33:[0-9]+]] = fir.convert %[[V_30]] : (!fir.heap<i64>) -> i64
+  ! CHECK:     %[[V_34:[0-9]+]] = arith.cmpi ne, %[[V_33]], %c0{{.*}} : i64
   ! CHECK:     fir.if %[[V_34]] {
   ! CHECK:       %[[V_45]] = fir.call @_FortranASystemClockCountMax(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
   ! CHECK:       fir.store %[[V_45]] to %[[V_30]] : !fir.heap<i64>
   ! CHECK:     }
-  ! CHECK:     %[[V_37]] = fir.call @_FortranAioBeginExternalListOutput
-  ! CHECK:     %[[V_38]] = fir.load %[[V_4]] : !fir.ref<!fir.ptr<i64>>
-  ! CHECK:     %[[V_39]] = fir.load %[[V_38]] : !fir.ptr<i64>
-  ! CHECK:     %[[V_40]] = fir.call @_FortranAioOutputInteger64(%[[V_37]], %[[V_39]]) {{.*}}: (!fir.ref<i8>, i64) -> i1
+  ! CHECK:     %[[V_37:[0-9]+]] = fir.call @_FortranAioBeginExternalListOutput
+  ! CHECK:     %[[V_38:[0-9]+]] = fir.load %[[V_4]] : !fir.ref<!fir.ptr<i64>>
+  ! CHECK:     %[[V_39:[0-9]+]] = fir.load %[[V_38]] : !fir.ptr<i64>
+  ! CHECK:     %[[V_40:[0-9]+]] = fir.call @_FortranAioOutputInteger64(%[[V_37]], %[[V_39]]) {{.*}}: (!fir.ref<i8>, i64) -> i1
   ! CHECK:     %[[V_41:[0-9]+]] = fir.load %[[V_1]] : !fir.ref<!fir.heap<i64>>
   ! CHECK:     %[[V_42:[0-9]+]] = fir.load %[[V_41]] : !fir.heap<i64>
   ! CHECK:     %[[V_43:[0-9]+]] = fir.call @_FortranAioOutputInteger64(%[[V_37]], %[[V_42]]) {{.*}}: (!fir.ref<i8>, i64) -> i1
@@ -120,13 +120,13 @@ subroutine ss(count)
   ! CHECK:     fir.store %[[V_29]] to %arg0 : !fir.ref<i64>
   ! CHECK:   }
   ! CHECK:   %[[V_24:[0-9]+]] = fir.convert %[[V_21]] : (!fir.ptr<i64>) -> i64
-  ! CHECK:   %[[V_25:[0-9]+]] = arith.cmpi ne, %[[V_24]], %c0{{.*}}_i64_1 : i64
+  ! CHECK:   %[[V_25:[0-9]+]] = arith.cmpi ne, %[[V_24]], %c0{{.*}} : i64
   ! CHECK:   fir.if %[[V_25]] {
   ! CHECK:     %[[V_29]] = fir.call @_FortranASystemClockCountRate(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
   ! CHECK:     fir.store %[[V_29]] to %[[V_21]] : !fir.ptr<i64>
   ! CHECK:   }
   ! CHECK:   %[[V_26:[0-9]+]] = fir.convert %[[V_22]] : (!fir.heap<i64>) -> i64
-  ! CHECK:   %[[V_27:[0-9]+]] = arith.cmpi ne, %[[V_26]], %c0{{.*}}_i64_2 : i64
+  ! CHECK:   %[[V_27:[0-9]+]] = arith.cmpi ne, %[[V_26]], %c0{{.*}} : i64
   ! CHECK:   fir.if %[[V_27]] {
   ! CHECK:     %[[V_29]] = fir.call @_FortranASystemClockCountMax(%c8{{.*}}_i32) {{.*}}: (i32) -> i64
   ! CHECK:     fir.store %[[V_29]] to %[[V_22]] : !fir.heap<i64>