Page MenuHomePhabricator

[MIPS] For vectors, select `add %x, C` as `sub %x, -C` if it results in inline immediate
AcceptedPublic

Authored by lebedev.ri on Tue, Aug 27, 8:09 AM.

Details

Summary

As discussed in https://reviews.llvm.org/D62341#1515637,
for MIPS add %x, -1 isn't optimal. Unlike X86 there
are no fastpaths to matearialize such -1/1 vector constants,
and sub %x, 1 results in better codegen,
so undo canonicalization

Diff Detail

Event Timeline

lebedev.ri created this revision.Tue, Aug 27, 8:09 AM

This patch targets very specific values(1,-1) but there are more. The trick for D62341 is that msa vector add/sub imm accept 5 bit unsigned imm, so it is ok to switch from add imm to sub imm (also sub to add) if it changes imm form negative to positive. I think it is cleanest to have some hook and ask target if it would like to transform sub into add (add to sub) with imm.

Thank you for taking a look.

This patch targets very specific values(1,-1) but there are more. The trick for D62341 is that msa vector add/sub imm accept 5 bit unsigned imm, so it is ok to switch from add imm to sub imm (also sub to add) if it changes imm form negative to positive.

Yeah, i have found that in ISA manual since posting the patch.

I think it is cleanest to have some hook and ask target if it would like to transform sub into add (add to sub) with imm.

I understand where this is coming from but no, that will not be ok.
The whole point of D62341 is that we really don't want sub %x, C in DAGCombine,
so while we could hide it behind target hook, that defies the whole purpose.
So this needs to be undone per-target in *ISelDAGToDAG.cpp.

Oh, I see. Then we have to undo combines here. Let's what for Simon.

Forgot to mention, this sub %X, C -> add %X, -C transform is already always being performed
by InstCombine in middle-end, so such undo transform is needed regardless of what DAGCombine does.

lebedev.ri retitled this revision from [MIPS] For vectors, select `add %x, -1` as `sub %x, 1` to [MIPS] For vectors, select `add %x, C` as `sub %x, -C` if it results in inline immediate.

Generalize the check, do not limit the transform to -1 specifically,
but rather all splat immediates if negating results in inline immediate (5-bit unsigned)

This revision is now accepted and ready to land.Tue, Sep 3, 5:14 AM

LGTM

Thank you for the review!