Enable the assertion checking that a BoxValue has an address and resides

in memory. The remaining issues were problems with the lowering of
CHARACTER variables and values. This patch attempts to clean up some of
that, passing CharBoxValue and Value where appropriate, etc.

Use llvm::dbgs() consistently.

Some clean up of other small TODOs and FIXMEs.

Author: schweitzpgi

flang/include/flang/Lower/CharacterExpr.h

@@ -53,7 +53,7 @@ class CharacterExprHelper {
 
   /// Lower \p lhs = \p rhs where \p lhs and \p rhs are scalar characters.
   /// It handles cases where \p lhs and \p rhs may overlap.
-  void createAssign(mlir::Value lhs, mlir::Value rhs);
+  void createAssign(mlir::Value lhs, const fir::ExtendedValue &rhs);
 
   /// Lower an assignment where the buffer and LEN parameter are known and do
   /// not need to be unboxed.
@@ -101,7 +101,7 @@ class CharacterExprHelper {
   materializeCharacterOrSequence(mlir::Value str);
 
   /// Return true if \p type is a character literal type (is
-  /// fir.array<len x fir.char<kind>>).;
+  /// `fir.array<len x fir.char<kind>>`).;
   static bool isCharacterLiteral(mlir::Type type);
 
   /// Return true if \p type is one of the following type
@@ -119,6 +119,7 @@ class CharacterExprHelper {
   /// Determine the base character type
   static fir::CharacterType getCharacterType(mlir::Type type);
   static fir::CharacterType getCharacterType(const fir::CharBoxValue &box);
+  static fir::CharacterType getCharacterType(mlir::Value str);
 
   /// Return the integer type that must be used to manipulate
   /// Character lengths. TODO: move this to FirOpBuilder?
@@ -130,10 +131,12 @@ class CharacterExprHelper {
   /// - fir.array<len x fir.char<kind>>
   /// - fir.char<kind>
   /// - fir.ref<char<kind>>
-  /// If the no length is passed, it is attempted to be extracted from \p
-  /// character (or its type). This will crash if this is not possible.
-  /// The returned value is a CharBoxValue if \p character is a scalar,
-  /// otherwise it is a CharArrayBoxValue.
+  ///
+  /// Does the heavy lifting of converting the value \p character (along with an
+  /// optional \p len value) to an extended value. If \p len is null, a length
+  /// value is extracted from \p character (or its type). This will produce an
+  /// error if it's not possible. The returned value is a CharBoxValue if \p
+  /// character is a scalar, otherwise it is a CharArrayBoxValue.
   fir::ExtendedValue toExtendedValue(mlir::Value character,
                                      mlir::Value len = {});
 
@@ -143,17 +146,23 @@ class CharacterExprHelper {
   ///   - !fir.array<dim x !fir.char<kind, len>>
   ///   - !fir.ref<T>  where T is either of the first two cases
   ///   - !fir.box<T>  where T is either of the first two cases
+  ///
+  /// In certain contexts, Fortran allows an array of CHARACTERs to be treated
+  /// as if it were one longer CHARACTER scalar, each element append to the
+  /// previous.
   static bool isArray(mlir::Type type);
 
 private:
-  fir::CharBoxValue materializeValue(const fir::CharBoxValue &str);
+  fir::CharBoxValue materializeValue(mlir::Value str);
   fir::CharBoxValue toDataLengthPair(mlir::Value character);
   mlir::Type getReferenceType(const fir::CharBoxValue &c) const;
+  mlir::Type getReferenceType(mlir::Value str) const;
   mlir::Type getSeqTy(const fir::CharBoxValue &c) const;
+  mlir::Type getSeqTy(mlir::Value str) const;
+  mlir::Value getCharBoxBuffer(const fir::CharBoxValue &box);
   mlir::Value createEmbox(const fir::CharBoxValue &str);
-  mlir::Value createLoadCharAt(const fir::CharBoxValue &str, mlir::Value index);
-  void createStoreCharAt(const fir::CharBoxValue &str, mlir::Value index,
-                         mlir::Value c);
+  mlir::Value createLoadCharAt(mlir::Value buff, mlir::Value index);
+  void createStoreCharAt(mlir::Value str, mlir::Value index, mlir::Value c);
   void createCopy(const fir::CharBoxValue &dest, const fir::CharBoxValue &src,
                   mlir::Value count);
   void createPadding(const fir::CharBoxValue &str, mlir::Value lower,

flang/include/flang/Lower/Support/BoxValue.h

@@ -54,8 +54,8 @@ class AbstractBox {
 public:
   AbstractBox() = delete;
   AbstractBox(mlir::Value addr) : addr{addr} {
-    //assert(isa_passbyref_type(addr.getType()) &&
-    //       "box values must be references");
+    assert(isa_passbyref_type(addr.getType()) &&
+           "box values must be references");
   }
 
   /// An abstract box always contains a memory reference to a value.
@@ -225,7 +225,11 @@ class ExtendedValue : public details::matcher<ExtendedValue> {
   using VT = std::variant<UnboxedValue, CharBoxValue, ArrayBoxValue,
                           CharArrayBoxValue, BoxValue, ProcBoxValue>;
 
-  template <typename A>
+  ExtendedValue() : box{UnboxedValue{}} {}
+  ExtendedValue(const ExtendedValue &) = default;
+  ExtendedValue(ExtendedValue &&) = default;
+  template <typename A, typename = std::enable_if_t<
+                            !std::is_same_v<std::decay_t<A>, ExtendedValue>>>
   constexpr ExtendedValue(A &&box) : box{std::forward<A>(box)} {}
 
   template <typename A>

flang/lib/Lower/Bridge.cpp

@@ -1484,8 +1484,10 @@ class FirConverter : public Fortran::lower::AbstractConverter {
               if (isCharacterCategory(lhsType->category())) {
                 // Fortran 2018 10.2.1.3 p10 and p11
                 // Generating value for lhs to get fir.boxchar.
+                Fortran::lower::ExpressionContext context;
                 auto lhs = genExprAddr(assign.lhs);
-                auto rhs = genExprValue(assign.rhs);
+                auto rhs = createSomeExtendedExpression(
+                    toLocation(), *this, assign.rhs, localSymbols, context);
                 Fortran::lower::CharacterExprHelper{*builder, loc}.createAssign(
                     lhs, rhs);
                 return;
@@ -1798,11 +1800,11 @@ class FirConverter : public Fortran::lower::AbstractConverter {
           // Assume that the members of the COMMON block will appear in an order
           // that is sorted by offset.
           [[maybe_unused]] std::int64_t lastByteOff = -1;
-          LLVM_DEBUG(llvm::errs() << "block {\n");
+          LLVM_DEBUG(llvm::dbgs() << "block {\n");
           for (const auto &obj : details->objects()) {
             assert(lastByteOff < static_cast<std::int64_t>(obj->offset()));
             lastByteOff = static_cast<std::int64_t>(obj->offset());
-            LLVM_DEBUG(llvm::errs() << "offset: " << obj->offset() << '\n');
+            LLVM_DEBUG(llvm::dbgs() << "offset: " << obj->offset() << '\n');
             if (const auto *objDet =
                     obj->detailsIf<Fortran::semantics::ObjectEntityDetails>())
               if (objDet->init()) {
@@ -1814,7 +1816,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
                                                         castVal, off);
               }
           }
-          LLVM_DEBUG(llvm::errs() << "}\n");
+          LLVM_DEBUG(llvm::dbgs() << "}\n");
           builder.create<fir::HasValueOp>(loc, cb);
         };
         global = builder->createGlobal(loc, commonTy, globalName,