Stream: git-cranelift

Topic: cranelift / Issue #1385 Add ineg legalization for scalar ...

GitHub (Feb 11 2020 at 10:25):

bjorn3 commented on Issue #1385:

Please also add one test for i8. Integer types <32bit often need extra legalizations.

GitHub (Feb 11 2020 at 11:24):

peterdelevoryas commented on Issue #1385:

Please also add one test for i8. Integer types <32bit often need extra legalizations.

Ya ok I have to admit defeat: I added an i8 test, and I tried debugging it, but it makes no sense: the ineg legalization of the i8 returns "done", not "legalized", presumably because it thinks that the ineg is already legal?? https://pastebin.com/0E1Bs06A I added print statements that trace the result of legalize_inst, and for the i32 case it returns "legalized" and then it legalizes an iconst and an isub, presumably the two instructions that get inserted by convert_ineg, but for the ineg of the i8 value it doesn't occur. This must have something to do with the ireduce, I just can't figure out what yet.

GitHub (Feb 12 2020 at 03:41):

peterdelevoryas commented on Issue #1385:

To summarize: everything is working except the i64 test I added, and I still have to remove the comments I added and add a better error message to the panic

GitHub (Feb 14 2020 at 16:14):

peterdelevoryas commented on Issue #1385:

Ok, so, I think the changes are finally done (ineg legalization tested on i686/x86_64 with all scalar integer types), one of my changes results in an unused import though

warning: unused import: `crate::ir::InstBuilder`
    --> /Users/peterd/src/peterd-cranelift/target/debug/build/cranelift-codegen-b0deee4e3070f0b5/out/legalize-x86.rs:1619:9
     |
1619 |     use crate::ir::InstBuilder;
     |         ^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `#[warn(unused_imports)]` on by default

This is the generated file:

/// Legalize instructions by narrowing.
///
/// Use x86-specific instructions if needed.
#[allow(unused_variables,unused_assignments,non_snake_case)]
pub fn x86_widen(
    inst: crate::ir::Inst,
    func: &mut crate::ir::Function,
    cfg: &mut crate::flowgraph::ControlFlowGraph,
    isa: &dyn crate::isa::TargetIsa,
) -> bool {
    use crate::ir::InstBuilder;
    use crate::cursor::{Cursor, FuncCursor};
    let mut pos = FuncCursor::new(func).at_inst(inst);
    pos.use_srcloc(inst);
    {
        match pos.func.dfg[inst].opcode() {
            ir::Opcode::Ineg => {
                convert_ineg(inst, func, cfg, isa);
                return true;
            }

            _ => {},
        }
    }
    crate::legalizer::widen(inst, func, cfg, isa)
}

I'm not sure exactly how to fix this, is there a way I could just add the custom legalization to the existing widen group without making a new x86_widen group? Maybe that would prevent the unused import?

GitHub (Feb 14 2020 at 16:16):

peterdelevoryas edited a comment on Issue #1385:

Ok, so, I think the changes are finally done (ineg legalization tested on i686/x86_64 with all scalar integer types), one of my changes results in an unused import though

warning: unused import: `crate::ir::InstBuilder`
    --> /Users/peterd/src/peterd-cranelift/target/debug/build/cranelift-codegen-b0deee4e3070f0b5/out/legalize-x86.rs:1619:9
     |
1619 |     use crate::ir::InstBuilder;
     |         ^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `#[warn(unused_imports)]` on by default

This is the generated file:

/// Legalize instructions by narrowing. <<< oops, noticed I forgot to fix this comment, fixed in the latest diff
///
/// Use x86-specific instructions if needed.
#[allow(unused_variables,unused_assignments,non_snake_case)]
pub fn x86_widen(
    inst: crate::ir::Inst,
    func: &mut crate::ir::Function,
    cfg: &mut crate::flowgraph::ControlFlowGraph,
    isa: &dyn crate::isa::TargetIsa,
) -> bool {
    use crate::ir::InstBuilder;
    use crate::cursor::{Cursor, FuncCursor};
    let mut pos = FuncCursor::new(func).at_inst(inst);
    pos.use_srcloc(inst);
    {
        match pos.func.dfg[inst].opcode() {
            ir::Opcode::Ineg => {
                convert_ineg(inst, func, cfg, isa);
                return true;
            }

            _ => {},
        }
    }
    crate::legalizer::widen(inst, func, cfg, isa)
}

I'm not sure exactly how to fix this, is there a way I could just add the custom legalization to the existing widen group without making a new x86_widen group? Maybe that would prevent the unused import?

GitHub (Feb 14 2020 at 21:16):

abrown commented on Issue #1385:

@peterdelevoryas, thanks!

GitHub (Feb 14 2020 at 21:18):

peterdelevoryas commented on Issue #1385:

Hey no thank you @abrown, and everyone else, for helping with this! I really got stuck on the whole transform group thing, but I think I get the idea now. I've really been enjoying using Cranelift, and it was fun making these changes!

Last updated: Dec 23 2024 at 14:03 UTC