Do not hash allocations to name them.#119458
Conversation
|
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
|
yeah, i agree that we should just let llvm deal with the unnamed consts lol r? compiler-errors @bors r+ rollup=never |
|
I added hashing of them to make diffing LLVM IR easier. |
…-errors Do not hash allocations to name them. Computing the stable hash behind an `Allocation` can be quite slow. The given name does not provide a lot of benefit: a hash is not meaningfully easier to understand than an arbitrary index.
|
@/bors treeclosed=100 |
|
💔 Test failed - checks-actions |
|
@bors retry Apple runner billing issue. |
This comment has been minimized.
This comment has been minimized.
|
You are also removing the |
|
@bjorn3: isn't that just because this is using the anonymous numbering from llvm? is there a reason to keep |
|
It shows up in the symbol table, right? Seeing just a symbol named 1234 would be less understandable what it is. Especially when decompiling where it could show up as |
|
@bjorn3 :
No, it does not. Those constants are "private unnamed_addr" for LLVM, and get no symbol in the binary. The only change is the name that appear in LLVM IR dumps. |
7177a04 to
cb6e396
Compare
|
@bors r+ |
|
🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened. |
…-errors Do not hash allocations to name them. Computing the stable hash behind an `Allocation` can be quite slow. The given name does not provide a lot of benefit: a hash is not meaningfully easier to understand than an arbitrary index.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
@rustbot author |
⬆️ That still seems like a valid argument? |
|
This PR is somehow back in the bors queue. It's not currently approved though. I don't know what's going on but I don't want bors to merge random PRs.^^ Since this is not the only affected PR... EDIT(jieyouxu): bors pls don't see this |
|
@bors r- Someone probably ran a resync? |
|
Yeah but that shouldn't screw everything up.^^ |
|
Right, I'm just pointing out that this is typically how PRs get back into the queue. I don't think anyone is gonna fix homu to stop doing that, though. |
|
Okay it seems it's just "PRs that were approved and then failed CI" that got re-queued. Not catastrophic then, they'll probably fail again.
EDIT(jieyouxu): bors pls don't see this |
|
☔ The latest upstream changes (presumably #123936) made this pull request unmergeable. Please resolve the merge conflicts. |
Computing the stable hash behind an
Allocationcan be quite slow. The given name does not provide a lot of benefit: a hash is not meaningfully easier to understand than an arbitrary index.