Page MenuHomePhabricator

[LiveDebugValues] 1/4 Install an implementation-picking LiveDebugValues pass
Needs ReviewPublic

Authored by jmorse on Jul 2 2020, 6:40 AM.

Details

Summary

(Context: this thread http://lists.llvm.org/pipermail/llvm-dev/2020-June/142368.html )

This patch series is preceeded by:

mkdir llvm/lib/CodeGen/LiveDebugValues
git mv llvm/lib/CodeGen/LiveDebugValues.cpp llvm/lib/CodeGen/VarLocBasedImpl.cpp

Plus moving the source file in CMakeLists.txt. I've assumed that such a file movement doesn't need reviewing; I can upload a review if anyone wants a closer look though.

This patch renames the current LiveDebugValues class to "InstrRefBasedLDV" and removes the pass-registration code from it. It creates a separate LiveDebugValues class that deals with pass registration and management, that calls through to InstrRefBasedLDV::ExtendRanges when runOnMachineFunction is called. This is done through the "LDVImpl" abstract class, so that a future patch can install the new instruction-referencing LiveDebugValues implementation and have it picked at runtime.

I'm not fixed to any particular symbol name in this, very happy to receive feedback on naming. I'm also aware of the "favour composition over inheritance" principle, but I believe this situation is exactly what abstract base classes (LDVImpl) are for.

Diff Detail

Event Timeline

jmorse created this revision.Jul 2 2020, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 6:41 AM
djtodoro added inline comments.Jul 2 2020, 7:34 AM
llvm/lib/CodeGen/CMakeLists.txt
184

We can add new subdirectory here as:
add_subdirectory(LiveDebugValues)

and then, add another CMakeLists.txt with in LiveDebugValues/ and play with all of this within that CMake file locally? wdyt?

Thanks a lot for this work!

jmorse marked an inline comment as done.Jul 2 2020, 8:21 AM
jmorse added inline comments.
llvm/lib/CodeGen/CMakeLists.txt
184

I've shied away from this due to being a CMake novice, but my understanding is we'd need to either:

  • Append the sources to the "LLVMCodeGen" library sources list from inside that new CMakeLists.txt, or
  • Define a new "llvm_component_library" for LiveDebugValues, which seems like overkill.

Referring to the source files from this CMakeList avoided me having to think about that; I can implement the first item if it's preferable for each directory to have a CMakeLists.txt.

vsk added a comment.Sun, Jul 5, 5:43 PM

Neat! Could you please clang-format the diff and add in license headers?

llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
52

Can this remain private?

70

nit, this seems simpler to just inline into the class definition?

djtodoro added inline comments.Mon, Jul 6, 12:36 AM
llvm/lib/CodeGen/CMakeLists.txt
184

I am OK with either way. If other folks do not have impression about that, this part is OK for me as well.

Referring to the source files from this CMakeList avoided me having to think about that; I can implement the first item if it's preferable for each directory to have a CMakeLists.txt.

I am not sure we need that if we do not consider the item 2.

Instead of git mv llvm/lib/CodeGen/LiveDebugValues.cpp llvm/lib/CodeGen/VarLocBasedImpl.cpp
it should be as following:
git mv llvm/lib/CodeGen/LiveDebugValues.cpp llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp

Plus moving the source file in CMakeLists.txt. I've assumed that such a file movement doesn't need reviewing; I can upload a review if anyone wants a closer look though.

I'd prefer adding all the patches in the stack, since there might be developers using some scripts for applying the patches automatically from Phabricator.

llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
2

Same as above.

23

According to the LLVM coding standard, this should be included first.

llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.h
2

I think we are missing the file header at the beginning of the file.

21

// namespace SharedLiveDebugValues

25

// namespace llvm

jmorse updated this revision to Diff 276001.Tue, Jul 7, 4:58 AM

clang-format, add file headers, address some review comments

jmorse marked 9 inline comments as done.Tue, Jul 7, 4:59 AM

Thanks for the feedback,

Instead of git mv llvm/lib/CodeGen/LiveDebugValues.cpp llvm/lib/CodeGen/VarLocBasedImpl.cpp
it should be as following:
git mv llvm/lib/CodeGen/LiveDebugValues.cpp llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp

Ah yep, missed that,

I'd prefer adding all the patches in the stack, since there might be developers using some scripts for applying the patches automatically from Phabricator.

Sure, I'll upload that shortly.

djtodoro added inline comments.Tue, Jul 7, 5:20 AM
llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
18

According to the LLVM coding standard, this should be included first.

Thanks for addressing comments! One nit included.

jmorse updated this revision to Diff 276036.Tue, Jul 7, 6:28 AM

Move include to start of file; was on autopilot sorry.

Super nits included.

llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp
13

I think this include is redundant.

llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp
1792

clang-format please

djtodoro added inline comments.Thu, Jul 9, 5:48 AM
llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.h
1

And since this is a header file we need:
-*- C++ -*-