bjorn3 commented on Issue #1113:
Most of cranelift-codegen-meta isn't used for the new backend framework anymore.
cfallin commented on Issue #1113:
Most of the issues here are related to the old backends, which will be removed eventually, and are fixed in the new backends; closing.
cfallin closed Issue #1113:
Having worked with cranelift for a few weeks now, I've had several discussions about how to improve the code in the
cranelift-codegen
module. I am sure that moving meta from Python to Rust has made the code easier to understand and use but here are some things I've noticed as I've worked in this module:
- __static references__:
cranelift-codegen
contains manyby_name
retrievals throughout the encodings, formats, legalization, etc. This means that when I add a new instruction, e.g., I can't encode it until I've found the alphabetically-ordered location to insert aby_name("my_inst")
--and I forget to do this every time. @bnjbvr mentioned that we might be able to use lazy_static to initialize them. Then we could use instructions, e.g., by importingcrate::shared
and writingshared::splat
where they are used. This may work for instructions, formats, recipes (maybe?), but we may run into issues with settings sinceSettingGroupBuilder::build
does more than just collect predicates.- __eliminate templates__:
cranelift-codegen
has the concept ofRecipe
s; the x86 subdirectory has the additional concept ofTemplate
s (previouslyTailRecipe
s). Perhaps I just don't understand the purpose ofTemplate
clearly enough (and would appreciate an explanation), but having bothRecipe
andTemplate
is very confusing; it is unclear when to build one or the other and the difference seems minimal. Is there a wayRecipe
can be improved to cover any features it is missing andTemplate
eliminated?- __recipe naming convention__: The x86 recipes have a confusing assortment of names: sometimes they make sense (
null
andnull_fpr
) but other times the naming is less than clear (e.g.brid
might stand for a branch recipe with a double-word immediate?). This makes it difficult to find existing recipes that may already do what I need. Can we explain the naming convention in the documentation and/or adopt a naming convention that is more explanatory?- __non-string recipes__: The recipe code itself (i.e. in
emit
) is difficult to change because it is a string, not Rust code. If a recipe is coded incorrectly, usually the build will fail; if the code was statically analyzable the errors would be clearer sooner. If insteademit
could be passed a closure or function to do the appropriate recipe work, then the code would be statically analyzable:.emit(|sink, bits, regs| { ... }
. This won't work as-is because each recipe uses different parameters and types, but perhaps a procedural macro wrapped around this (i.e.emit(recipe_from!(|sink, bits, regs| { ... })
) could reduce a real Rust closure/function to the stringsemit
expects while allowing IDEs to statically analyze the code.- __re-factor PerCpuModeEncodings__: The x86 encodings use a
PerCpuModeEncodings
struct to join an instruction, its bound types, its encodings, its recipes, etc. Currently there are a confusing assortment of methods for adding different subsets of these (e.g. enc_x86_64_instp, enc_both, enc_x86_64, enc_i32_i64, etc.). Perhaps a builder-like pattern would allow us to remove some of these methods and clarify what is happening; something like:e.inst(band).with_types([B1, B8, B16]).uses_recipe(rec_rr.opcodes(...)).when_isa([use_sse3]).when_predicate(...).encode();
. For instructions that have several encodings following a pattern (i.e. like that ofenc_i32_i64
) we could add a.encode_with_pattern(...)
accepting an enum for the various patterns.I am opening this issue to discuss if any of the above are feasible and desirable. Please add to this list if there are other items.
Last updated: Dec 23 2024 at 12:05 UTC