cfallin commented on issue #4485:
Additionally, change the name argument for LoadExtAddr from a Box<ExternalName> to ExternalName, as it greatly simplifies the interface with ISLE, and there didn't appear to be a downside.
Can you check the size of the resulting
Inst
enum? That's likely the reason we did the boxing; larger insts have measurable and pretty significant compile-performance implications (higher cache footprint, more memory traffic).
github-actions[bot] commented on issue #4485:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
elliottt commented on issue #4485:
Additionally, change the name argument for LoadExtAddr from a Box to ExternalName, as it greatly simplifies the interface with ISLE, and there didn't appear to be a downside.
Can you check the size of the resulting
Inst
enum? That's likely the reason we did the boxing; larger insts have measurable and pretty significant compile-performance implications (higher cache footprint, more memory traffic).That change didn't affect the size of the x64
Inst
, however as it was already 72 bytes I went ahead and switched back toBoxExternalName
to not add extra work for whoever decides to start optimizing for size.
elliottt edited a comment on issue #4485:
Can you check the size of the resulting
Inst
enum? That's likely the reason we did the boxing; larger insts have measurable and pretty significant compile-performance implications (higher cache footprint, more memory traffic).That change didn't affect the size of the x64
Inst
, however as it was already 72 bytes I went ahead and switched back toBoxExternalName
to not add extra work for whoever decides to start optimizing for size.
elliottt edited a comment on issue #4485:
Can you check the size of the resulting
Inst
enum? That's likely the reason we did the boxing; larger insts have measurable and pretty significant compile-performance implications (higher cache footprint, more memory traffic).That change didn't affect the size of the x64
Inst
, however as it was already 72 bytes I went ahead and switched back toBoxExternalName
to not add extra work for whoever decides to start optimizing for size. I also made another PR that adds a test for x64 instruction size so that we can avoid slipping further #4489.
Last updated: Dec 23 2024 at 12:05 UTC