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

FEXCore: Moves CodeEmitter to own module #3622

Merged
merged 5 commits into from
May 17, 2024

Conversation

Sonicadvance1
Copy link
Member

@Sonicadvance1 Sonicadvance1 commented May 8, 2024

The frontend needs to have a microJIT for BPF/seccomp. It can't use the CodeEmitter from FEXCore in its current state.

  • Removes the vixl dependency from the CodeEmitter
    • Only vixl uses were its Float16 class. Replaced with a custom implementation
    • And IsImmLogical, which was added to its own header with vixl's license maintained. Reimplementing the logic is hard.
  • Moves it to its own module
  • Renames the namespace

No functional change

@Sonicadvance1 Sonicadvance1 force-pushed the move_emitter branch 2 times, most recently from 1d90594 to 6b1a0f8 Compare May 8, 2024 19:31
static inline int CountLeadingZeros(V value, int width = (sizeof(V) * 8)) {
#if COMPILER_HAS_BUILTIN_CLZ
if (width == 32) {
return (value == 0) ? 32 : __builtin_clz(static_cast<unsigned>(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this ifdef and fallback can be replaced with std::countl_zero from the <bit> header

Copy link
Member Author

@Sonicadvance1 Sonicadvance1 May 8, 2024

Choose a reason for hiding this comment

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

These two helpers are directly pulled from vixl. Didn't want to mess with them especially since they have some prickly behaviours like passing in a uint8_t but casting to different widths.

In-fact everything in this particular file is from vixl with as little changes as possible.


template <typename V>
static inline bool IsPowerOf2(V value) {
return (value != 0) && ((value & (value - 1)) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

std::has_single_bit

@neobrain
Copy link
Member

neobrain commented May 9, 2024

Why doesn't this live in its own module? FHU makes sense for things that heavily inter-depend on each other and that are used virtually everywhere, but this is a fairly isolated piece of code.

@Sonicadvance1
Copy link
Member Author

Why doesn't this live in its own module? FHU makes sense for things that heavily inter-depend on each other and that are used virtually everywhere, but this is a fairly isolated piece of code.

Moved to its own module.

@Sonicadvance1 Sonicadvance1 changed the title FEXCore: Moves CodeEmitter to FHU FEXCore: Moves CodeEmitter to own module May 9, 2024
@neobrain
Copy link
Member

Looks fine to me in the grand scheme now that there's a dedicated CMake target.

Only vixl uses were its Float16 class. Replaced with a custom implementation

Is there an upside to this other than making the dependency graph simpler (without removing the use of vixl elsewhere)? The new class has no unit tests, so its upsides must be weighed against this added risk and the additional time required for the more careful review.

@Sonicadvance1
Copy link
Member Author

Looks fine to me in the grand scheme now that there's a dedicated CMake target.

Only vixl uses were its Float16 class. Replaced with a custom implementation

Is there an upside to this other than making the dependency graph simpler (without removing the use of vixl elsewhere)? The new class has no unit tests, so its upsides must be weighed against this added risk and the additional time required for the more careful review.

An upside is that the vixl dependency isn't leaked to the frontend.

@neobrain
Copy link
Member

An upside is that the vixl dependency isn't leaked to the frontend.

This could instead be achieved by hiding the definition of Float16 in a private cpp file. We can then make vixl a PRIVATE CMake dependency, such that its include directories don't get propagated to the frontend.

@Sonicadvance1
Copy link
Member Author

To what end? This is the easiest route.

@neobrain
Copy link
Member

To what end? This is the easiest route.

The PR replaces battle-proven code with a custom reimplementation of the same interface. Continuing to use vixl avoids risks associated with that, though I'd be happy to instead see tests for the new Float16 code as well.

@Sonicadvance1
Copy link
Member Author

At this point I might as well as just delete all Float16 references since we don't even use it.

@neobrain
Copy link
Member

At this point I might as well as just delete all Float16 references since we don't even use it.

I didn't know the code using Float16 wasn't hit. If it can just be dropped, that's even better.

@Sonicadvance1 Sonicadvance1 force-pushed the move_emitter branch 2 times, most recently from e2204f2 to b803573 Compare May 13, 2024 11:17
@Sonicadvance1
Copy link
Member Author

At this point I might as well as just delete all Float16 references since we don't even use it.

I didn't know the code using Float16 wasn't hit. If it can just be dropped, that's even better.

Deleted.

Creating local Float16 helper which handles our needs
To ensure we round everything correctly for the new float16 class
The only core vixl usage we use in the emitter. Is a complete pain to
reimplement so keep it around.
Now that the vixl dependency is gone, this gets moved to FHU since the
frontend is going to need it for a microjit.
We aren't using it. We won't be using it. We need unit tests in our
lives if we want this.
@Sonicadvance1 Sonicadvance1 merged commit 048c8de into FEX-Emu:main May 17, 2024
11 checks passed
@Sonicadvance1 Sonicadvance1 deleted the move_emitter branch May 17, 2024 17:41
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