[flang-commits] [PATCH] D147239: [flang] Fix context less NULL() lowering

[Jean Perier via Phabricator via flang-commits]

jeanPerier created this revision.
jeanPerier added reviewers: vzakhari, clementval.
jeanPerier added a project: Flang.
Herald added subscribers: sunshaoce, mehdi_amini, jdoerfert.
Herald added a project: All.
jeanPerier requested review of this revision.

The current context less lowering of NULL is producing invalid code
(can lead to reading outside of allocated memory): it is casting
a simple pointer to a descriptor address.
Later, reads are made to this descriptor. It used to be "OK" when
fir.load of fir.box were no-ops, but this was incorrect, and the
fir.load codegen is known doing a copy, and read the whole descriptor
data, not only the base address.

The previous patch that allowed fir.box<None> allocation, this
code fix this by allocating an actual fir.box<None>.

Note: this is still an overkill way to lower foo(null()). HLFIR
lowering always contextualize NULL() lowering leading to much simpler
code:

  %absent = fir.absent fir.box<T>
  fir.call @foo(%absent)

Depends on: D147237 <https://reviews.llvm.org/D147237>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147239

Files:
  flang/lib/Lower/ConvertExpr.cpp
  flang/test/Lower/dummy-argument-optional.f90
  flang/test/Lower/polymorphic.f90


Index: flang/test/Lower/polymorphic.f90
===================================================================
--- flang/test/Lower/polymorphic.f90
+++ flang/test/Lower/polymorphic.f90
@@ -843,15 +843,19 @@
   end subroutine
 
 ! CHECK-LABEL: func.func @_QMpolymorphic_testPtest_call_with_null() {
-! CHECK: %[[NULL_LLVM_PTR:.*]] = fir.alloca !fir.llvm_ptr<none>
-! CHECK: %[[NULL_BOX_NONE:.*]] = fir.convert %[[NULL_LLVM_PTR]] : (!fir.ref<!fir.llvm_ptr<none>>) -> !fir.ref<!fir.box<none>>
-! CHECK: %[[BOX_NONE:.*]] = fir.load %[[NULL_BOX_NONE]] : !fir.ref<!fir.box<none>>
-! CHECK: %[[BOX_ADDR:.*]] = fir.box_addr %[[BOX_NONE]] : (!fir.box<none>) -> !fir.ref<none>
-! CHECK: %[[BOX_ADDR_I64:.*]] = fir.convert %[[BOX_ADDR]] : (!fir.ref<none>) -> i64
+! CHECK: %[[NULL_PTR:.*]] = fir.alloca !fir.box<!fir.ptr<none>>
+! CHECK: %[[NULL:.*]] = fir.zero_bits !fir.ptr<none>
+! CHECK: %[[NULL_BOX:.*]] = fir.embox %[[NULL]] : (!fir.ptr<none>) -> !fir.box<!fir.ptr<none>>
+! CHECK: fir.store %[[NULL_BOX]] to %[[NULL_PTR]] : !fir.ref<!fir.box<!fir.ptr<none>>>
+! CHECK: %[[BOX_NONE:.*]] = fir.load %[[NULL_PTR]] : !fir.ref<!fir.box<!fir.ptr<none>>>
+! CHECK: %[[BOX_ADDR:.*]] = fir.box_addr %[[BOX_NONE]] : (!fir.box<!fir.ptr<none>>) -> !fir.ptr<none>
+! CHECK: %[[BOX_ADDR_I64:.*]] = fir.convert %[[BOX_ADDR]] : (!fir.ptr<none>) -> i64
 ! CHECK: %[[C0:.*]] = arith.constant 0 : i64
 ! CHECK: %[[IS_ALLOCATED_OR_ASSOCIATED:.*]] = arith.cmpi ne, %[[BOX_ADDR_I64]], %[[C0]] : i64
 ! CHECK: %[[ABSENT:.*]] = fir.absent !fir.class<none>
-! CHECK: %[[BOX_NONE:.*]] = fir.load %[[NULL_BOX_NONE]] : !fir.ref<!fir.box<none>>
+! CHECK: %[[PTR_LOAD2:.*]] = fir.load %[[NULL_PTR]] : !fir.ref<!fir.box<!fir.ptr<none>>>
+! CHECK: %[[BOX_ADDR2:.*]] = fir.box_addr %[[PTR_LOAD2]] : (!fir.box<!fir.ptr<none>>) -> !fir.ptr<none>
+! CHECK: %[[BOX_NONE:.*]] = fir.embox %[[BOX_ADDR2]] : (!fir.ptr<none>) -> !fir.box<none>
 ! CHECK: %[[CLASS_NONE:.*]] = fir.convert %[[BOX_NONE]] : (!fir.box<none>) -> !fir.class<none>
 ! CHECK: %[[ARG:.*]] = arith.select %[[IS_ALLOCATED_OR_ASSOCIATED]], %[[CLASS_NONE]], %[[ABSENT]] : !fir.class<none>
 ! CHECK: fir.call @_QMpolymorphic_testPsub_with_poly_optional(%[[ARG]]) {{.*}} : (!fir.class<none>) -> ()
Index: flang/test/Lower/dummy-argument-optional.f90
===================================================================
--- flang/test/Lower/dummy-argument-optional.f90
+++ flang/test/Lower/dummy-argument-optional.f90
@@ -195,9 +195,10 @@
 
 ! CHECK-LABEL: func @_QMoptPnull_as_optional() {
 subroutine null_as_optional
-  ! CHECK: %[[temp:.*]] = fir.alloca !fir.llvm_ptr<none>
-  ! CHECK: %[[null:.*]] = fir.zero_bits !fir.ref<none>
-  ! CHECK: fir.store %{{.*}} to %[[temp]] : !fir.ref<!fir.llvm_ptr<none>>
+  ! CHECK: %[[null_ptr:.*]] = fir.alloca !fir.box<!fir.ptr<none>>
+  ! CHECK: %[[null:.*]] = fir.zero_bits !fir.ptr<none>
+  ! CHECK: %[[null_box:.*]] = fir.embox %[[null]] : (!fir.ptr<none>) -> !fir.box<!fir.ptr<none>>
+  ! CHECK: fir.store %[[null_box]] to %[[null_ptr]] : !fir.ref<!fir.box<!fir.ptr<none>>>
   ! CHECK: fir.call @_QMoptPassumed_shape(%{{.*}}) {{.*}}: (!fir.box<!fir.array<?xf32>>) -> ()
  call assumed_shape(null())
 end subroutine null_as_optional
Index: flang/lib/Lower/ConvertExpr.cpp
===================================================================
--- flang/lib/Lower/ConvertExpr.cpp
+++ flang/lib/Lower/ConvertExpr.cpp
@@ -711,20 +711,22 @@
   }
 
   /// A `NULL()` in a position where a mutable box is expected has the same
-  /// semantics as an absent optional box value.
+  /// semantics as an absent optional box value. Note: this code should
+  /// be depreciated because the rank information is not known here. A
+  /// scalar fir.box is created: it should not be cast to an array box type
+  /// later, but there is no way to enforce that here.
   ExtValue genMutableBoxValueImpl(const Fortran::evaluate::NullPointer &) {
     mlir::Location loc = getLoc();
-    auto nullConst = builder.createNullConstant(loc);
-    auto noneTy = mlir::NoneType::get(builder.getContext());
-    auto polyRefTy = fir::LLVMPointerType::get(noneTy);
-    // MutableBoxValue will dereference the box, so create a bogus temporary for
-    // the `nullptr`. The LLVM optimizer will garbage collect the temp.
-    auto temp =
-        builder.createTemporary(loc, polyRefTy, /*shape=*/mlir::ValueRange{});
-    auto nullPtr = builder.createConvert(loc, polyRefTy, nullConst);
-    builder.create<fir::StoreOp>(loc, nullPtr, temp);
+    mlir::Type noneTy = mlir::NoneType::get(builder.getContext());
+    mlir::Type polyRefTy = fir::PointerType::get(noneTy);
+    mlir::Type boxType = fir::BoxType::get(polyRefTy);
+    mlir::Value nullConst = builder.createNullConstant(loc, polyRefTy);
+    mlir::Value tempBox =
+        builder.createTemporary(loc, boxType, /*shape=*/mlir::ValueRange{});
+    mlir::Value nullBox = builder.create<fir::EmboxOp>(loc, boxType, nullConst);
+    builder.create<fir::StoreOp>(loc, nullBox, tempBox);
     auto nullBoxTy = builder.getRefType(fir::BoxType::get(noneTy));
-    return fir::MutableBoxValue(builder.createConvert(loc, nullBoxTy, temp),
+    return fir::MutableBoxValue(tempBox,
                                 /*lenParameters=*/mlir::ValueRange{},
                                 /*mutableProperties=*/{});
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147239.509688.patch
Type: text/x-patch
Size: 5297 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/flang-commits/attachments/20230330/5e293c45/attachment-0001.bin>