Add ineg legalization for scalar integer types

[peterdelevoryas]

• Closes ineg instruction on ssa values: register affinity never assigned? #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

[bjorn3]

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

[peterdelevoryas]

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.

[peterdelevoryas]

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

[abrown]

@bjorn3, I would not expect this iconcat; can't we directly encode an iconst.i64?

[bjorn3]

This file has target i686 before target x86_64. How is that handled? Does it test both or only the first?

[abrown]

Learn something new every day: apparently parsing tests accumulates the targets and test_tuples will expand the different targets (in this case since the legalizer needs_isa we should have a test tuple for each target from here).

But then this doesn't make sense: how could these checks pass when target x86_64 is set? Is that the build failure? No, it looks like the current build failure happens because ineg isn't getting legalized at all for i64?

[peterdelevoryas]

Yeah the build failure is happening because ineg still isn't getting legalized for i64, I still haven't figured out why that is. This must be because I need to extend another transform group right? That was the reason for i8 and i16 not getting legalized initially (I think)

[bjorn3]

There is also the narrow group for ints > pointer size.

[peterdelevoryas]

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?

[abrown]

Perhaps we should rename this to x86_widen (here and where used) to be very clear about what is going on.

[peterdelevoryas]

Oh yeah I can definitely do that!

[abrown]

Sounds good, I'll merge this after your push.

[abrown]

@peterdelevoryas, thanks!

[peterdelevoryas]

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!