Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add bolt #22204
base: main
Are you sure you want to change the base?
add bolt #22204
Changes from 25 commits
8886325
e9097a6
b888381
4d9a06b
c7c344f
dd2dbeb
12d9023
46b34e2
350e74c
aedb4ee
61cc902
f79e231
dca8d45
d39eba5
19aebfc
828a41b
c605794
e51d279
53b59f6
cb2f1c4
f8d0234
0e96961
11472ee
eebba24
99ba39a
6bb8bdc
a54de91
bd1a6f1
004cdc9
4f17e53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about include files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had been wondering about that too, but there's nothing I can find. The only occurrences of
include/bolt
in the logs (example) are during the build, not during installation. I've now done a regex-search (include/.*bolt
) as well, and it only finds false positivesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's anything that goes beyond what's in
llvmdev
, it's now being installed (4f17e53)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no include files, then we might as well drop
libbolt-devel
as users can't use those libraries (not easily anyways).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know sufficiently well how bolt works, but it's conceivable to me that bolt the binary needs those libraries to actually transform a binary (perhaps embedding some symbols from the support libraries along the way).
It seemed natural to split off the libraries, but if you want, I can just stuff everything into
bolt
itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check that the cmake files are here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only way to do that currently is to clobber
from llvmdev, unfortunately. But I guess better than nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bolt doesn't have different cmake files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, it's built as part of llvm. I guess we could ask for separate CMake files in the "standalone" upstream PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change your opinion somehow, or are you fine with the
always_include_files:
(0e96961)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isuruf, could you let me know how you'd like to proceed with the CMake metadata here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make
libbolt-devel
depend onllvmdev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'm planning to merge this once CI passes (well, on unix at least; the rest I'll fix on the feedstock).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't. We require reviews from everyone including core team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then please don't drip-feed change requests. If there's been two rounds of review and no more open comments are left, it's not unreasonable to think that things are wrapped up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two should be in
bolt
as they are used at runtime ofbolt
. The others are only needed if bolt is used as a library.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure (between the two open threads) what you want me to do now. Here you say
But in the other you say
I understood that we'll move
libbolt_rt_{hugify,instr}
tobolt
. But for the rest, do we want to:libbolt-devel
bolt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 is preferred. 1 if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I found the bolt headers: https://github.com/llvm/llvm-project/tree/main/bolt/include/bolt; no idea why they're not getting installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I guess we can restrict this to linux until anything changes upstream w.r.t. llvm/llvm-project#72205