This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Thumb2: ConstantMaterializationCost
ClosedPublic

Authored by SjoerdMeijer on Jan 28 2019, 7:19 AM.

Details

Summary

Constants can also be materialised using the negated value and a MVN, and this
case seem to have been missed for Thumb2. To check the constant materialisation
costs, we now call getT2SOImmVal twice, once for the original constant and then
also for its negated value, and this function checks if the constant can both
be splatted or rotated.

This was revealed by a test that optimises for minsize: instead of a LDR
literal pool load and having a literal pool entry, just a MVN with an immediate is
smaller (and also faster). This gives a small code size improvement for all our
apps in our code corpus, only 2 test cases have a tiny, insignificant
regression.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Jan 28 2019, 7:19 AM

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

lib/Target/ARM/ARMISelDAGToDAG.cpp
453–454

I don't think the change from getT2SOImmValSplatVal to getT2SOImmVal makes much practical difference; isThumbImmShiftedVal covers the same cases. But I guess it's more clear, at least.

Maybe fix the MOV/MVN/MOVW comments so they're each on the same line as the corresponding check?

test/CodeGen/ARM/shifter_operand.ll
262 ↗(On Diff #183854)

You can put the attributes inline here (instead of "#0", just write "optsize minsize").

SjoerdMeijer added inline comments.Jan 29 2019, 1:10 AM
lib/Target/ARM/ARMISelDAGToDAG.cpp
453–454

Forgot to mention that, but indeed, I found there was no practical difference, but thought it is clearer.

test/CodeGen/ARM/shifter_operand.ll
262 ↗(On Diff #183854)

Thanks, didn't know that!

Addressed comments.

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

Maybe put the tests into test/CodeGen/ARM/subtarget-no-movt.ll? We have some similar tests there from https://reviews.llvm.org/D23090 .

Ah, sorry, somehow I missed that comment.

  • The test cases have been moved to test/CodeGen/ARM/subtarget-no-movt.ll, and
  • while I was at it, cleaned up the duplicated mess that was this file (and it also included a typo USE-MOVTT-O0 resulting in FileCheck not checking that line, one day I would really like to change FileCheck so that it errors for these cases...)
efriedma accepted this revision.Jan 30 2019, 11:31 AM

LGTM with one nit.

lib/Target/ARM/ARMISelDAGToDAG.cpp
454

The "+" here is misleading (especially given other nearby comments); either replace it with a "/", or put the checks on separate lines.

This revision is now accepted and ready to land.Jan 30 2019, 11:31 AM
SjoerdMeijer marked an inline comment as done.Jan 31 2019, 12:19 AM
SjoerdMeijer added inline comments.
lib/Target/ARM/ARMISelDAGToDAG.cpp
454

Thanks, will do before committing.

This revision was automatically updated to reflect the committed changes.