abrown requested cfallin and bnjbvr for a review on PR #2468.
abrown opened PR #2468 from improve-constants
to main
:
This adds a higher-level method,
LowerCtx::emit_constant
for lowering a constant in one of two ways:
- using a
DataValue
(instead of the previousu64
), a value can be generated into a register usingMachInst::gen_constant
- alternately, if
MachInst::should_genenerate_constant_in_pool
is true, then a value is emitted into the constant pool/island and loaded from memory at runtime; this path usesMachInst::gen_constant_in_pool
.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
abrown requested cfallin and bnjbvr for a review on PR #2468.
abrown updated PR #2468 from improve-constants
to main
:
This adds a higher-level method,
LowerCtx::emit_constant
for lowering a constant in one of two ways:
- using a
DataValue
(instead of the previousu64
), a value can be generated into a register usingMachInst::gen_constant
- alternately, if
MachInst::should_genenerate_constant_in_pool
is true, then a value is emitted into the constant pool/island and loaded from memory at runtime; this path usesMachInst::gen_constant_in_pool
.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin submitted PR Review.
cfallin submitted PR Review.
cfallin created PR Review Comment:
s/in place/into a constant pool/, no? (Or perhaps "should be generated into a constant pool and loaded (if true); otherwise, it should be generated in place by an instruction or instruction sequence."
We should also clarify that if this function returns
false
, thengen_constant
must be able to handle this value, and iftrue
, thengen_constant_in_pool
should.
cfallin created PR Review Comment:
Perhaps we can call this
gen_constant_in_place
now (analogous to ..._in_pool
below)?
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Looks like there's a solution involving more traits that could avoid the assertions and the dummy unimplemented:
- have a trait
ConstantGenerator
that has a method "gen_constant"- have a trait
PooledConstantGenerator
, that has three methods "gen_constant_without_pool", "gen_constant_with_pool", "must_generate_in_pool"- then
PooledConstantGenerator
gets an blanket impl ofConstantGenerator
based on these three functions above.Then backends have to implement
ConstantGenerator
either directly, or via an opt-in choice to use the pooled variant. It's a bit more involved though...
bnjbvr edited PR Review Comment.
bnjbvr submitted PR Review.
cfallin submitted PR Review.
cfallin created PR Review Comment:
I like this scheme -- @abrown, does this seem reasonable to incorporate in your PR? I'd be happy to review it as a refactoring followup otherwise.
abrown submitted PR Review.
abrown created PR Review Comment:
I like it in principle! Let me dive back in and see how it comes out in practice.
abrown closed without merge PR #2468.
Last updated: Dec 23 2024 at 13:07 UTC