Skip to content

Conversation

h-vetinari
Copy link
Contributor

This is a build regression from 1f4d91e when using -DLLVM_USE_INTEL_JITEVENTS=ON (already commented by @Zentrik on the commit), as std::map is evidently used further down in that TU

using NativeCodeMap = std::map<uint64_t, SourceLocations>;

CC @kazutakahirata

h-vetinari referenced this pull request Nov 28, 2024
@Zentrik
Copy link
Contributor

Zentrik commented Nov 28, 2024

Perhaps we should only include map if -DLLVM_USE_INTEL_JITEVENTS=ON like in https://github.com/llvm/llvm-project/pull/117926/files.

@h-vetinari
Copy link
Contributor Author

Fine by me to put it behind the #if as well - I just put it where it had been pre-1f4d91e

@h-vetinari
Copy link
Contributor Author

@kazutakahirata, since your commit broke compilation here, can you please let us know how you'd prefer to be fixed?

Copy link
Contributor

@kazutakahirata kazutakahirata left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for being late in getting back to this.

@h-vetinari
Copy link
Contributor Author

Thanks for the review. Any thoughts on whether this should move behind the #if LLVM_USE_INTEL_JITEVENTS?

Also, as I don't have the commit rights, I cannot merge this myself - would appreciate if someone could push the button. :)

@h-vetinari
Copy link
Contributor Author

Ping @kazutakahirata - given that this your cleanup (at least partially automated AFAIU?), it would be good to ensure that this doesn't happen again for the next clean-up. If moving this behind the respective #if would help, I'm happy to do it. If not, then the PR can IMO be merged as-is (I just don't have the rights to merge myself).

@h-vetinari
Copy link
Contributor Author

Ping @kazutakahirata; please either merge this PR (I don't have the commit bit), or - preferably - respond to my question above.

@h-vetinari
Copy link
Contributor Author

Ping @kazutakahirata

@h-vetinari
Copy link
Contributor Author

Based on

#### MCJIT, Orc, RuntimeDyld, PerfJITEvents
Lang Hames \
[email protected] (email), [lhames](https://github.com/lhames) (GitHub)

perhaps @lhames can chime in here (or simply merge)? Not sure whose maintainership this falls under, but I'm not successful in getting a response here.

@kazutakahirata
Copy link
Contributor

Ping @kazutakahirata - given that this your cleanup (at least partially automated AFAIU?), it would be good to ensure that this doesn't happen again for the next clean-up. If moving this behind the respective #if would help, I'm happy to do it. If not, then the PR can IMO be merged as-is (I just don't have the rights to merge myself).

Sorry again for being so late in getting back to you. Yes, moving the include behind #if LLVM_USE_INTEL_JITEVENTS sounds good. Let me create PR for that.

@kazutakahirata
Copy link
Contributor

I've created #124083.

@h-vetinari
Copy link
Contributor Author

Yes, moving the include behind #if LLVM_USE_INTEL_JITEVENTS sounds good.

It would have been a trivial update to this PR, but whatever gets this fixed. ;-)

@h-vetinari
Copy link
Contributor Author

Obsoleted by #124083

@h-vetinari h-vetinari closed this Jan 23, 2025
@h-vetinari h-vetinari deleted the orc_jit_map branch January 23, 2025 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants