This is an archive of the discontinued LLVM Phabricator instance.

[XRay] [compiler-rt] Move machine-dependent code into machine-dependent files.
ClosedPublic

Authored by pelikan on Oct 6 2016, 10:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pelikan updated this revision to Diff 73885.Oct 6 2016, 10:10 PM
pelikan retitled this revision from to [XRay] Move machine-dependent code into machine-dependent files..
pelikan updated this object.
pelikan added a reviewer: dberris.
pelikan retitled this revision from [XRay] Move machine-dependent code into machine-dependent files. to [XRay] [NFC] Move machine-dependent code into machine-dependent files..Oct 6 2016, 10:18 PM
pelikan updated this object.
pelikan added a subscriber: cfe-commits.
pelikan retitled this revision from [XRay] [NFC] Move machine-dependent code into machine-dependent files. to [XRay] [NFC] [compiler-rt] Move machine-dependent code into machine-dependent files..Oct 6 2016, 10:30 PM
pelikan edited subscribers, added: llvm-commits; removed: cfe-commits.Oct 6 2016, 10:34 PM
dberris requested changes to this revision.Oct 6 2016, 10:36 PM
dberris edited edge metadata.
dberris added inline comments.
lib/xray/xray_arm.cc
23 ↗(On Diff #73885)

Style guide recommends functions start with lowercase letters.

37 ↗(On Diff #73885)

Same here.

This revision now requires changes to proceed.Oct 6 2016, 10:36 PM
pelikan updated this revision to Diff 73886.Oct 6 2016, 10:46 PM
pelikan edited edge metadata.

style guide fixes

pelikan marked 2 inline comments as done.Oct 6 2016, 10:48 PM
dberris requested changes to this revision.Oct 9 2016, 5:24 PM
dberris edited edge metadata.

So, these additional function calls replacing the in-lined code will incur non-trivial overheads in the fast path. As discussed offline we should do this refactoring differently or at a smaller scale.

This revision now requires changes to proceed.Oct 9 2016, 5:24 PM
pelikan updated this revision to Diff 76104.Oct 27 2016, 2:45 PM
pelikan edited edge metadata.

[XRay] [NFC] [compiler-rt] Move machine-dependent code into machine-dependent files.

pelikan updated this revision to Diff 76114.Oct 27 2016, 3:16 PM
pelikan edited edge metadata.

[XRay] [NFC] [compiler-rt] Move machine-dependent code into machine-dependent files.

pelikan updated this revision to Diff 76291.Oct 28 2016, 9:18 PM

[XRay] [NFC] [compiler-rt] Move machine-dependent code into machine-dependent files.

pelikan updated this revision to Diff 76292.Oct 28 2016, 9:46 PM

[XRay] [NFC] [compiler-rt] Move machine-dependent code into machine-dependent files.

will incur non-trivial overheads in the fast path. As discussed offline
we should do this refactoring differently or at a smaller scale.

Fixed. The fast path is now more clear and marked as never-instrument to for bootstrapping LLVM with XRay enabled.

dberris requested changes to this revision.Oct 31 2016, 6:11 PM
dberris edited edge metadata.
dberris added inline comments.
lib/xray/xray_arm.cc
23 ↗(On Diff #76292)

Probably want constexpr here.

lib/xray/xray_inmemory_log.cc
141 ↗(On Diff #76292)

We cannot actually assume that the compiler used to build compiler-rt actually supports this attribute. Let's defer this for later, when we have a check in CMake to figure out whether the compiler we're using supports XRay explicitly, then make this conditional on a macro.

lib/xray/xray_x86_64.h
6 ↗(On Diff #76292)

Consider adding an attribute that forces this to be always inline, if not have a //TODO: or //FIXME: that indicates so.

This revision now requires changes to proceed.Oct 31 2016, 6:11 PM
pelikan updated this revision to Diff 81868.Dec 17 2016, 8:33 PM
pelikan edited edge metadata.
pelikan marked 3 inline comments as done.

I've still kept this as a NFC change, despite the wrong strncat usage and Report() format strings. It's mostly shuffling code around.

I'm still waiting on an ARM build machine, so tested only on amd64.

dberris requested changes to this revision.Dec 18 2016, 5:21 PM
dberris edited edge metadata.
dberris added inline comments.
lib/xray/xray_inmemory_log.cc
108 ↗(On Diff #81868)

Probably want to add XRAY_NEVER_INSTRUMENT as an attribute, because of weird recursion problems.

lib/xray/xray_x86_64.cc
66 ↗(On Diff #81868)

Did you miss an \n in the string?

This revision now requires changes to proceed.Dec 18 2016, 5:21 PM
pelikan updated this revision to Diff 81902.Dec 18 2016, 5:30 PM
pelikan edited edge metadata.
pelikan marked 2 inline comments as done.

fix newline and a never instrument annotation

dberris requested changes to this revision.Dec 18 2016, 5:52 PM
dberris edited edge metadata.

Just a few more...

lib/xray/xray_emulate_tsc.h
8 ↗(On Diff #81902)

You can't mark this always inline as well? Also probably want the never instrument attribute here.

20 ↗(On Diff #81902)

Same here, always inline and never instrument?

lib/xray/xray_x86_64.cc
56 ↗(On Diff #81902)

Always inline, never instrument?

lib/xray/xray_x86_64.h
8 ↗(On Diff #81902)

Also never instrument?

This revision now requires changes to proceed.Dec 18 2016, 5:52 PM
pelikan updated this revision to Diff 81906.Dec 18 2016, 6:04 PM
pelikan edited edge metadata.
pelikan marked 4 inline comments as done.

annotated functions as requested

dberris accepted this revision.Dec 18 2016, 6:07 PM
dberris edited edge metadata.
This revision is now accepted and ready to land.Dec 18 2016, 6:07 PM

Local testing implies that there's a build break with this change.

../lib/clang/4.0.0/lib/linux/libclang_rt.xray-x86_64.a(xray_inmemory_log.cc.o): In function `__xray_OpenLogFile':
.../llvm/projects/compiler-rt/lib/xray/xray_inmemory_log.cc:138: undefined reference to `__xray::cycleFrequency()'
clang-4.0: error: linker command failed with exit code 1 (use -v to see invocation)
dberris requested changes to this revision.Dec 18 2016, 6:31 PM
dberris edited edge metadata.

Please fix before I land?

This revision now requires changes to proceed.Dec 18 2016, 6:31 PM
pelikan updated this revision to Diff 81908.Dec 18 2016, 6:49 PM
pelikan edited edge metadata.

remove ALWAYS_INLINE from a function which is forward-declared

pelikan updated this revision to Diff 81913.Dec 18 2016, 7:22 PM
pelikan edited edge metadata.

ARM cycleFrequency() moved to .cc just like amd64 has it.

pelikan retitled this revision from [XRay] [NFC] [compiler-rt] Move machine-dependent code into machine-dependent files. to [XRay] [compiler-rt] Move machine-dependent code into machine-dependent files..Dec 18 2016, 7:23 PM
dberris accepted this revision.Dec 18 2016, 7:24 PM
dberris edited edge metadata.

Thanks @pelikan -- trying again now.

This revision is now accepted and ready to land.Dec 18 2016, 7:24 PM
This revision was automatically updated to reflect the committed changes.