Stream: cranelift

Topic: Small code review of osa1/mincaml

bjorn3 (May 24 2020 at 10:31):

@osa1 I just took a look at the codegen part of your mincaml. Here are a few things I noticed:

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L28

// How does this know I'm building for x86_64 Linux?
cranelift_native detects the host architecture and returns it.

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L30

[1, 2, 3, 4, 5, 6, 7, 8], // TODO: what is this?
The "name" of the object file. It is mainly used in linker errors.

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L161

call_conv: CallConv::SystemV,
You may want to use module.target_config().default_call_conv instead. Also you may want to use module.target_config().pointer_type() above instead of I64.

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L236-L237

// TODO: We already created a signature for this function, in the forward declaration in
// init_module_env. Is there a way to reuse it here?
No, the signature that you assign to a Function gets modified as part of the compilation process. For example the register location gets assigned or types bigger than a register get split into multiple arguments

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L265

let entry_block = *label_to_block.get(&blocks[0].label).unwrap();
You can use let entry_block = label_to_block[&blocks[0].label];.

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L349

// Not sure about the arguments here...
This should be &[] like you have here unless you are doing something special.

https://github.com/osa1/mincaml/blob/b8ef0a735cc5a88333f7aebaf89a5be062bebf03/src/codegen.rs#L356

let cl_block = *label_to_block.get(label).unwrap();
Same as with entry_block.

osa1 (May 24 2020 at 10:45):

Oh wow, I got a review from a cranelift dev!! Thank you so much! I'll update the code.

osa1 (May 24 2020 at 10:46):

Btw, I'm currently writing my own code generator, and the data structures in entity are so useful, I'm basically copying all of them to my code base. I wish they were in a reusable package. They don't mention cranelift types anyway so this would be easy, I think

osa1 (May 24 2020 at 10:47):

Oh wait, it's already a package with no deps!!!

osa1 (May 24 2020 at 11:07):

Hmm.. it's a bit unforunate that I need Eq for ReservedValue. I wonder if it would be better to have a is_reserved_value() method in the trait instead. I don't need full Eq to check for reserved values.

bjorn3 (May 24 2020 at 11:08):

osa1 said:

Oh wow, I got a review from a cranelift dev!! Thank you so much! I'll update the code.

I do contribute to Cranelift, but I am not a member of the bytecodealliance. I am mostly a consumer of Cranelift myself. (https://github.com/bjorn3/rustc_codegen_cranelift)

bjorn3 (May 24 2020 at 11:10):

osa1 said:

Hmm.. it's a bit unforunate that I need Eq for ReservedValue. I wonder if it would be better to have a is_reserved_value() method in the trait instead. I don't need full Eq to check for reserved values.

I think that would also work.

osa1 (May 24 2020 at 11:19):

Also, SecondaryMap<..., PackedOption<X>> shouldn't require X: Clone, I think.

bjorn3 (May 24 2020 at 11:22):

In that case you could would be able to do SecondaryMap::with_default(PackedOption::from(Some(NoClone))), which would give problems.

osa1 (May 24 2020 at 11:23):

What's the problem with it?

bjorn3 (May 24 2020 at 11:25):

In that case SecundaryMap would have to clone the PackedOption<NoClone> when it needs a new copy of the default element. However that type can't be cloned, as it contains a NoClone, which can't be cloned.

osa1 (May 24 2020 at 11:29):

Right, I mean, it should ideally designed in a way that doesn't require X: Clone for the type above, as we don't need to full general Clone for X, we only need to clone the default value for X. Perhaps that's not possible for some reason though.

osa1 (May 24 2020 at 11:31):

Hmm, is it possible to provide a specialized implementation, like impl SecondaryMap<K, PackedOption<V>> ? That wouldn't require V: Clone

bjorn3 (May 24 2020 at 11:31):

Because of the existence of SecundaryMap::with_default, any value can be the default value.

osa1 (May 24 2020 at 11:31):

Hmm, right

bjorn3 (May 24 2020 at 11:32):

osa1 said:

Hmm, is it possible to provide a specialized implementation, like impl SecondaryMap<K, PackedOption<V>> ? That wouldn't require V: Clone

That will probably require a differently names set of methods to prevent conflicts. Or needs to use #![feature(specialization)], which is unsound and nightly only. (not sure if #![feature(min_specialization)] is enough)

osa1 (May 24 2020 at 11:59):

This type checks:

impl<K, V> SecondaryMap<K, PackedOption<V>>
where
    K: EntityRef,
    V: ReservedValue,
{
    /// blah blah blah
    pub fn new_reserved() -> Self {
        Self {
            elems: Vec::new(),
            default: Default::default(),
            unused: PhantomData,
        }
    }

    /// blah blah blah
    pub fn resize_reserved(&mut self, n: usize)
    where
    {
        self.elems.resize_with(n, || Default::default());
    }
}

osa1 (May 24 2020 at 11:59):

The default field is not used

bjorn3 (May 24 2020 at 12:15):

How are you going to implement IndexMut for SecundaryMap<K, PackedOption<V>>? It overlaps with the implementation for SecundaryMap<K, V>.

osa1 (May 24 2020 at 12:16):

I don't need to implement it, the implemented version will work, right?

osa1 (May 24 2020 at 12:17):

Oh.. you're right.

osa1 (May 24 2020 at 12:17):

Yeah those need to be overridden as well.. sigh.

osa1 (May 24 2020 at 12:29):

I guess this is not a problem in cranelift as seconday maps also have a primary map for the actual values (double indirection, e.g. SecondaryMap<Block, Option<BlockIndex>>) or the actual values are simple types.

bjorn3 (May 24 2020 at 12:32):

Most of the time Cranelift uses SecondaryMap either for simple types or vectors of simple types.

bjorn3 (May 24 2020 at 12:32):

What is the specific case you want to use SecondaryMap for?

osa1 (May 24 2020 at 12:34):

I want to map block indices (an entity) to actual block contents (complex type that has a vector of instructions), but some of the blocks may not be used after allocation so I need a SecondaryMap for that.

bjorn3 (May 24 2020 at 12:41):

If a lot of blocks are unused you can use SparseMap. Otherwise see if there is some sensible default value for the block content type and use a PrimaryMap. Or use Option<Block> as value. The Block type contains a Vec, which has a niche (invalid value, in this case a null pointer), so layout optimizations should use that niche as None value.

osa1 (May 24 2020 at 13:02):

Yeah PrimaryMap<K, Option<V>> works, though it's not as nice ergonomically

Last updated: Oct 23 2024 at 20:03 UTC