Page MenuHomePhabricator

Transform 3 operand instructions to 2 operand versions of the same instruction if first 2 register operands are the same for thumb1
ClosedPublic

Authored by rs on Sep 23 2014, 4:43 AM.

Details

Summary

This patch makes the ARM backend transform 3 operand instructions such as 'adds' to the 2 operand version of the same instruction if the first two register operands are the same e.g. 'adds r0, r0, #1' will is transformed to 'adds r0, #1'. Currently for some instructions such as 'adds' if you try to assemble 'adds r0, r0, #8' for thumb v6m the assembler would throw an error message because the immediate cannot be encoded using 3 bits, the backend should be smart enough to transform the instruction to 'adds r0, #8'.

Diff Detail

Event Timeline

rs updated this revision to Diff 13983.Sep 23 2014, 4:43 AM
rs retitled this revision from to Transform 3 operand instructions to 2 operand versions of the same instruction if first 2 register operands are the same for thumb1.
rs updated this object.
rs edited the test plan for this revision. (Show Details)
rs added reviewers: t.p.northover, rengolin.
rs set the repository for this revision to rL LLVM.
rs added a subscriber: Unknown Object (MLST).
mroth added a subscriber: mroth.Sep 23 2014, 7:36 AM
rengolin added inline comments.Sep 24 2014, 3:12 AM
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
5657

Either call these Reg1, Reg2 or Op3, Op4.

5668

Being pedantic, the ARM ARM refers to the first register as being optional, not the second. :)

5672

why is this necessary?

5688

You could simplify this with a nested if

8196

Can you describe why is this change needed?

rs added inline comments.Sep 24 2014, 6:56 AM
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
5657

ok

5672

The 3 operand 'add' is transformed to the 2 operand 'add' and the 2 operand register-register 'add' does not have a CCOut operand, so it must be removed (see line 5236 which says the same thing).

5688

I'm not sure what you mean, would you prefer this 'if' to be put into nested 'if''s, or do you want it to be part of the previous 'if'?

8196

If you try to assemble 'add r1, r2' (echo 'add r1, r2' | llvm-mc --show-encoding --triple thumbv6m) you would get the error message ' error: instruction variant requires Thumb2', this instruction is supported in v6m.

rengolin added inline comments.Sep 26 2014, 5:41 AM
lib/Target/ARM/AsmParser/ARMAsmParser.cpp
5672

ok

5688

I mean something like:

f (isThumbOne() && Operands.size() == 5 && Mnemonic == "add" && !CarrySetting) {
    ARMOperand Op2 = static_cast<ARMOperand &>(*Operands[2]);
    ARMOperand Op3 = static_cast<ARMOperand &>(*Operands[3]);
    if (Op2.isReg() && Op3.isReg() && Op1 == Op3)
        Operands.erase(Operands.begin() + 2);
}
8196

oh, ok, I read it backwards.

rs updated this revision to Diff 14117.Sep 26 2014, 7:13 AM

Hi Renato,

I've attached a new patch with changes that you suggested, could you do another review please?

Thanks

rengolin accepted this revision.Sep 26 2014, 7:42 AM
rengolin edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Sep 26 2014, 7:42 AM
rs added a comment.Sep 26 2014, 7:45 AM

I don't have commit rights yet so could you commit this patch for me?

Thanks

rengolin closed this revision.Sep 26 2014, 9:26 AM

Committed in r218521. Thanks.