[clang] [CIR] Match the parameter spill shape instead of asserting it (PR #224977)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Adam Smith (adams381)

<details>
<summary>Changes</summary>

Return null from findParamSpill when a non-byval indirect parameter's only use is not a store into an alloca it names directly, instead of asserting that shape.  Both callers already handled a null result, so an unmatched parameter now reaches the diagnostic the forwarding call already has for an operand that does not name the caller's storage.


---
Full diff: https://github.com/llvm/llvm-project/pull/224977.diff


3 Files Affected:

- (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp (+15-13) 
- (added) clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-param-spill-nyi.cpp (+30) 
- (added) clang/test/CIR/Transforms/abi-lowering/non-byval-param-spill-nyi.cir (+71) 


``````````diff
diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp
index 371fa67922872..1debf88aabfe3 100644
--- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp
+++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp
@@ -558,22 +558,24 @@ static void eraseDeadRecordLoads(ArrayRef<cir::LoadOp> loads) {
       load->erase();
 }
 
-/// The store that spills non-byval indirect parameter \p blockArg, and the
-/// slot it spills into.  CIRGen spills every by-value parameter into a local
-/// alloca with a single store before any other use, and this pass runs on that
-/// CIRGen output before any alloca-promoting or splitting pass, so the block
-/// argument has exactly that one use.  Both results are null when DCE already
-/// removed a dead spill.
+/// The store that spills non-byval indirect parameter \p blockArg into a local
+/// alloca it names directly, and that alloca.  Both are null unless the block
+/// argument's only use is such a store.  CIRGen emits the spill as that only
+/// use, but an earlier pass can add more: CIRSimplify replaces each read of
+/// the const slot a const-qualified parameter is spilled to with the stored
+/// value, so the block argument then feeds the spill and every one of those
+/// readers.
 static std::pair<cir::StoreOp, cir::AllocaOp>
 findParamSpill(mlir::BlockArgument blockArg) {
-  if (blockArg.use_empty())
+  if (!blockArg.hasOneUse())
     return {};
-  assert(blockArg.hasOneUse() &&
-         "non-byval arg must have exactly one use (the CIRGen param spill)");
-  auto store = cast<cir::StoreOp>(*blockArg.user_begin());
-  assert(store.getValue() == blockArg &&
-         "non-byval arg's use must be the value operand of its store");
-  return {store, cast<cir::AllocaOp>(store.getAddr().getDefiningOp())};
+  auto store = dyn_cast<cir::StoreOp>(*blockArg.user_begin());
+  if (!store || store.getValue() != blockArg)
+    return {};
+  auto slot = dyn_cast_or_null<cir::AllocaOp>(store.getAddr().getDefiningOp());
+  if (!slot)
+    return {};
+  return {store, slot};
 }
 
 /// For each Direct arg with a coerced type, change the block argument's type
diff --git a/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-param-spill-nyi.cpp b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-param-spill-nyi.cpp
new file mode 100644
index 0000000000000..2f73140d20a94
--- /dev/null
+++ b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-param-spill-nyi.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O1 -fclangir -emit-cir \
+// RUN:     %s -o /dev/null -verify
+
+// -O1 is load-bearing twice: Impl's key function is not defined here, so the
+// vtable and its thunk are only emitted when optimizing, and CIRSimplify,
+// which folds the read of the const slot, only runs then.
+
+struct NonTrivial {
+  ~NonTrivial();
+};
+
+struct HasVirtual {
+  virtual void h();
+};
+
+struct Middle : HasVirtual {};
+
+struct Renderer {
+  virtual void draw(NonTrivial);
+};
+
+// Renderer sits at a non-zero offset in Impl, so overriding draw needs a
+// this-adjusting thunk.
+struct Impl : Middle, Renderer {
+  // expected-error at +2 {{does not name the caller's storage}}
+  // expected-note at +1 {{see current operation}}
+  void draw(const NonTrivial);
+};
+
+void emit() { new Impl; }
diff --git a/clang/test/CIR/Transforms/abi-lowering/non-byval-param-spill-nyi.cir b/clang/test/CIR/Transforms/abi-lowering/non-byval-param-spill-nyi.cir
new file mode 100644
index 0000000000000..9b3cd37a8a957
--- /dev/null
+++ b/clang/test/CIR/Transforms/abi-lowering/non-byval-param-spill-nyi.cir
@@ -0,0 +1,71 @@
+// RUN: not cir-opt %s -split-input-file \
+// RUN:     -cir-call-conv-lowering="classification-attr=test_classify" \
+// RUN:     2>&1 | FileCheck %s
+
+!s64i = !cir.int<s, 64>
+!rec_Big = !cir.struct<"Big" {data !s64i, data !s64i, data !s64i, data !s64i}>
+
+#non_byval_arg = {
+  return = { kind = "direct" },
+  args   = [ { kind = "indirect", indirect_align = 8, byval = false } ]
+}
+
+module attributes {
+  dlti.dl_spec = #dlti.dl_spec<
+    #dlti.dl_entry<i64, dense<64>: vector<2xi64>>>
+} {
+
+  cir.func private @takes_big(%arg0: !rec_Big)
+      attributes { test_classify = #non_byval_arg }
+
+  // A non-byval parameter with no spill at all: its only use is the
+  // forwarding call, so there is no load to look through and no slot to point
+  // the callee at.
+  cir.func @forwards_unspilled(%unspilled: !rec_Big)
+      attributes { test_classify = #non_byval_arg } {
+    cir.call @takes_big(%unspilled) : (!rec_Big) -> ()
+    cir.return
+  }
+
+  // CHECK: error: 'cir.call' op non-byval indirect argument that
+  // CHECK-SAME: does not name the caller's storage is not yet
+  // CHECK-SAME: implemented in CallConvLowering
+  // CHECK-NEXT: cir.call @takes_big(%unspilled)
+
+}
+
+// -----
+
+!s64i = !cir.int<s, 64>
+!rec_Big = !cir.struct<"Big" {data !s64i, data !s64i, data !s64i, data !s64i}>
+
+#non_byval_arg = {
+  return = { kind = "direct" },
+  args   = [ { kind = "indirect", indirect_align = 8, byval = false } ]
+}
+
+module attributes {
+  dlti.dl_spec = #dlti.dl_spec<
+    #dlti.dl_entry<i64, dense<64>: vector<2xi64>>>
+} {
+
+  cir.func private @takes_big(%arg0: !rec_Big)
+      attributes { test_classify = #non_byval_arg }
+
+  // A parameter both spilled and forwarded, as CIRSimplify leaves it after
+  // folding a read of a const parameter's slot: the spill is one of two uses
+  // rather than the only one.
+  cir.func @spills_and_forwards(%spilled: !rec_Big)
+      attributes { test_classify = #non_byval_arg } {
+    %slot = cir.alloca "spilled" align(8) init const : !cir.ptr<!rec_Big>
+    cir.store %spilled, %slot : !rec_Big, !cir.ptr<!rec_Big>
+    cir.call @takes_big(%spilled) : (!rec_Big) -> ()
+    cir.return
+  }
+
+  // CHECK: error: 'cir.call' op non-byval indirect argument that
+  // CHECK-SAME: does not name the caller's storage is not yet
+  // CHECK-SAME: implemented in CallConvLowering
+  // CHECK-NEXT: cir.call @takes_big(%spilled)
+
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/224977