Skip to content
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

[BOLT][NFC] Add allocator id to createInstrIncMemory #68709

Closed
wants to merge 2 commits into from

Conversation

mtvec
Copy link
Contributor

@mtvec mtvec commented Oct 10, 2023

On RISC-V, this function will have to insert auipc+lw/sw pairs where the auipc needs a label annotation (see #66395). In order to do this correctly within the context of ParallelUtilities, we need an allocator id when setting the label. This patch adds this allocator id argument and wires it through to all call sites.

Note that this function is also made non-const since setLabel is non-const.

On RISC-V, this function will have to insert `auipc`+`lw`/`sw` pairs
where the `auipc` needs a label annotation (see llvm#66395). In order to do
this correctly within the context of `ParallelUtilities`, we need an
allocator id when setting the label. This patch adds this allocator id
argument and wires it through to all call sites.

Note that this function is also made non-const since `setLabel` is
non-const.
@yota9
Copy link
Member

yota9 commented Oct 10, 2023

Overall LG, but probably needs riscv implementation, since by it's own seems to be meaningless..

@mtvec
Copy link
Contributor Author

mtvec commented Oct 11, 2023

Overall LG, but probably needs riscv implementation, since by it's own seems to be meaningless..

That will be part of the PR implementing instrumentation for RISC-V. Still waiting for a few dependent patches to land before submitting that one.

Copy link
Contributor

@rafaelauler rafaelauler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI instrumentation pass is not running in parallel

@mtvec
Copy link
Contributor Author

mtvec commented Oct 19, 2023

FYI instrumentation pass is not running in parallel

This really confused me but now I see that ForceSequential=true is passed here.

Is it guaranteed that this will always be the case? If so, this PR isn't necessary.

Just for my information: why is ParallelUtilities even used if everything is executed sequentially?

@rafaelauler
Copy link
Contributor

At the time I tought it would be good to have it ready to run in parallel because I couldn't tell how expensive it would be to run the instrumentation pass. But eventually I realized I had little gains from running this in parallel so I just turned it off, but kept the scaffolding for parallel execution "just in case" we need to revisit this. I know realize this was silly, and we can just take that off.

@mtvec
Copy link
Contributor Author

mtvec commented Nov 2, 2023

Thanks for the explanation, @rafaelauler! I think this can be closed now, especially once #70147 gets merged.

@mtvec mtvec closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants