Page MenuHomePhabricator

[NFC] Define move and copy constructors and copy assignment operators for MDOperand.
ClosedPublic

Authored by wolfgangp on May 19 2022, 11:25 AM.

Details

Summary

Preparation for a patch to introduce a resizing functionality for MDNodes. See D124548 for a more detailed discussion.

Using a SmallVector<MDOperand> as a large storage container seems to require these constructors/operators to be defined.

Diff Detail

Unit TestsFailed

TimeTest
60,050 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp

Event Timeline

wolfgangp created this revision.May 19 2022, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:25 AM
wolfgangp requested review of this revision.May 19 2022, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:25 AM

Can you add a unit test for this? Ideally something that could expose errors if the tracking logic isn’t right, so probably a TempMDTuple or something assigned/moved between operands.

llvm/include/llvm/IR/Metadata.h
778

There shouldn’t be a const here, and Op should probably be reset. Is there a retrack() function for that operation?

782

I don’t think we need this and it seems better to error here. I think we should keep the copy constructor/operator deleted and make this move-only.

dexonsmith added inline comments.May 19 2022, 12:54 PM
llvm/include/llvm/IR/Metadata.h
793

Looking at this, is it even safe to use the standard move operators? Seems like you don’t have a way to pass in the owner…

wolfgangp added inline comments.May 19 2022, 2:34 PM
llvm/include/llvm/IR/Metadata.h
782

In that case I'm a bit stuck because the implementation of SmallVector uses std::fill_n() and std::uninitialized_fill_n() which assign and copy the value type and want the copy constructor (at least on gcc-9). Do you have any suggestions of should we roll our own hung-off storage class after all?

793

IIUC (and I might be wrong there), only operands of uniqued nodes have an owner? Since we're only resizing non-uniqued nodes it seems it would probably work but how would one enforce the destination node always being non-uniqued?

dexonsmith added inline comments.May 19 2022, 4:04 PM
llvm/include/llvm/IR/Metadata.h
778

Yeah, there is; TrackingMDRef(TrackingMDRef &&X) seems to call it.

782

I suspect that's because the move constructor/operator had the wrong signature (an incorrect const). The following seems to work for me locally:

struct MoveOnly {
  MoveOnly() = default;
  MoveOnly(MoveOnly &&) = default;
  MoveOnly &operator=(MoveOnly &&) = default;
  MoveOnly(const MoveOnly &) = delete;
  MoveOnly &operator=(const MoveOnly &) = delete;
};
SmallVector<MoveOnly> X;
X.resize(10);
X.push_back(MoveOnly());
793

Looks like MDNode doesn't expose non-const MDOperand& so it's probably safe as-is!

wolfgangp added inline comments.May 19 2022, 4:59 PM
llvm/include/llvm/IR/Metadata.h
782

Ah, I had the equivalent of

SmallVector<MoveOnly> X(NumOps);

which doesn't compile for me. Anyway that's the workaround.

dexonsmith added inline comments.May 19 2022, 6:52 PM
llvm/include/llvm/IR/Metadata.h
782

Huh! We should probably fix that to make it work at some point! Curious that one works and not the other.

wolfgangp updated this revision to Diff 431173.May 21 2022, 1:15 PM

Only define the move constructor and move assignment operator. Use retrack() instead of relying on the destructor to untrack the source op.

Can you add a unit test for this? Ideally something that could expose errors if the tracking logic isn’t right, so probably a TempMDTuple or something assigned/moved between operands.

(Not sure if you were posting for review yet, but I took a look and noticed this comment hadn’t been addressed yet.)

wolfgangp updated this revision to Diff 431480.May 23 2022, 2:19 PM

Added a unit test to make sure the move constructor and the move assignment op of MDOperand adjust tracking info.

This revision is now accepted and ready to land.May 23 2022, 4:39 PM