================
@@ -426,28 +444,36 @@ emitStructFieldArgs(mlir::OpBuilder &builder,
mlir::Location loc,
mlir::Value structVal, cir::RecordType recTy,
SmallVectorImpl<mlir::Value> &newArgs,
SmallVectorImpl<cir::LoadOp> &replacedWholeLoads) {
- cir::LoadOp wholeLoad = structVal.getDefiningOp<cir::LoadOp>();
- cir::AllocaOp srcAlloca;
- if (wholeLoad && !wholeLoad.getIsVolatile() && !wholeLoad.getMemOrder())
- srcAlloca = wholeLoad.getAddr().getDefiningOp<cir::AllocaOp>();
+ WholeRecordSource src = getWholeRecordSource(structVal);
- if (srcAlloca) {
+ if (src.alloca) {
mlir::OpBuilder::InsertionGuard guard(builder);
- builder.setInsertionPoint(wholeLoad);
+ builder.setInsertionPoint(src.load);
for (auto [f, fieldTy] : llvm::enumerate(recTy.getMembers())) {
mlir::Type fieldPtrTy = cir::PointerType::get(fieldTy);
mlir::Value fieldPtr = cir::GetMemberOp::create(
- builder, loc, fieldPtrTy, srcAlloca, /*name=*/"", /*index=*/f);
+ builder, loc, fieldPtrTy, src.alloca, /*name=*/"", /*index=*/f);
newArgs.push_back(cir::LoadOp::create(builder, loc, fieldPtr));
}
- replacedWholeLoads.push_back(wholeLoad);
+ replacedWholeLoads.push_back(src.load);
} else {
for (unsigned f = 0; f < recTy.getNumElements(); ++f)
newArgs.push_back(
cir::ExtractMemberOp::create(builder, loc, structVal, f));
}
}
+/// Erase the whole-record loads a call-site rewrite read around, once the
+/// original call (their remaining user) is gone. A single load can feed
+/// several operands (e.g. after CSE merges identical loads), so dedupe before
+/// erasing to avoid touching a freed op twice.
+static void eraseDeadWholeRecordLoads(ArrayRef<cir::LoadOp> loads) {
+ SmallPtrSet<mlir::Operation *, 4> erased;
+ for (cir::LoadOp wholeLoad : loads)
+ if (erased.insert(wholeLoad).second && wholeLoad.use_empty())
----------------
adams381 wrote:
I've rewritten this to try and make it more clear. I've renamed some things in
other places so that the code should flow more clearly.
In the code you were looking at the `erased` set was really tracking which
loads were already visited, not which ones had been erased, so the name was
confusing. In that code checking `use_empty()` first would have been a use
after free. The same load can be in the list twice, once per operand it feeds,
like `f(s, s)` with both operands byref, which is the
`caller_two_byref_one_load` test. The first visit erases the load, and
`erase()` deletes the op, so the second visit would be asking a deleted op
about its uses. The insert-first ordering was the only thing preventing that.
Trying to reconstruct that explanation from that conditional was too much to
ask of a reader of the code.
I've now pulled the dedupe out in front of the loop. Each load gets visited
once now, and `use_empty()` is the only thing in the conditional. Additionally
I've added a test, `caller_byref_then_direct`, where the load survives because
a Direct operand still reads it.
https://github.com/llvm/llvm-project/pull/216499
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
