Page MenuHomePhabricator

[Thumb1] Any imm of i8 type on Thumb1 should have cost of 1
ClosedPublic

Authored by zzheng on Sep 18 2018, 7:42 PM.

Details

Summary

A simple <MOVS rd, imm8> can materialize [-128, 127] in signed i8 type or [0, 255] in unsigned i8 type on Thumb1.

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng created this revision.Sep 18 2018, 7:42 PM

I think the test file can use a bit of clean up: we don't the attributes, metadata, etc. But more importantly, can it perhaps be further reduced? Do we need all this code?

Ah, but looking a bit closer now, I am not sure this is the right thing to do. This changes makes any 8 bit value cheap, including negative numbers. And I am not sure if this is the right thing to do, since the Thumb1 immediates are positive numbers. It looks this a workaround for store-merging interacting badly with constant hoisting.

Is this because we can just use a MOVS and wont have to fill in any higher bits? And MOVS's aren't trivially rematerialisable? And Thumb2/Arm are handled by getT2SOImmVal?

So, yeah, looks sensible to me, and this does seem to reduce codesize. But please clean up the testcase.

zzheng updated this revision to Diff 166570.Sep 21 2018, 3:52 PM
zzheng edited the summary of this revision. (Show Details)

Cleaned up test case.

zzheng added a comment.EditedSep 21 2018, 3:58 PM

Ah, but looking a bit closer now, I am not sure this is the right thing to do. This changes makes any 8 bit value cheap, including negative numbers. And I am not sure if this is the right thing to do, since the Thumb1 immediates are positive numbers. It looks this a workaround for store-merging interacting badly with constant hoisting.

Depends on how it's interpreted, 0xFF is -1 in signed i8 or 255 in unsigned i8.
Right, constant hoisting breaks store merging for this case.

Is this because we can just use a MOVS and wont have to fill in any higher bits? And MOVS's aren't trivially rematerialisable? And Thumb2/Arm are handled by getT2SOImmVal?

So, yeah, looks sensible to me, and this does seem to reduce codesize. But please clean up the testcase.

Yes, since it's i8 type, no need to deal with higher bits. We ran into this case with internal workload, it's intended to reduce code size.

dmgreen accepted this revision.Sep 24 2018, 3:07 AM

Sounds good. I'm happy with this, if no one else has any issues.

This revision is now accepted and ready to land.Sep 24 2018, 3:07 AM

Yep, sounds good, cheers.

This revision was automatically updated to reflect the committed changes.