Page MenuHomePhabricator

[ARM][test] Add more tests of two-part immediates
ClosedPublic

Authored by benshi001 on Jul 18 2020, 7:24 AM.

Details

Summary

The ARM backend breaks some specific immediates to two parts
in binary operations. And this patch adds more tests
for that.

Diff Detail

Event Timeline

benshi001 created this revision.Jul 18 2020, 7:24 AM

This patch does two optimization:

  1. mov Rx, 0x1ffff

before:

ldr
...
.long ...

after:

mov Rx, # 0x20000
sub Rx, Rx, 1
  1. add Rx, Rx, # 0x1ffff

before

ldr
add
...
.long ...

after:

add Rx, # 0x20000
sub Rx, # 1

The patch depends on https://reviews.llvm.org/D83928, it will show improvements based on it.

benshi001 updated this revision to Diff 279075.Jul 19 2020, 5:59 AM
benshi001 edited the summary of this revision. (Show Details)
benshi001 updated this revision to Diff 284204.Aug 9 2020, 7:44 AM
samparker added inline comments.Aug 10 2020, 5:18 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
3319

NewUseOpc2 = NewUseOpc1, same for below.

3365

NewUseOpc2 = NewUseOpc1, same for below.

llvm/test/CodeGen/ARM/add-sub-imm.ll
2

We should have a test run for a Thumb2 target as well. It would also be a good idea for some edge-case negative tests to exercise isSOImmTwoPartValSub some more.

benshi001 updated this revision to Diff 284571.Aug 10 2020, 8:42 PM
benshi001 marked 3 inline comments as done.

I have added tests for thumb2, however current patch does not apply to thumb2.

I will submit another patch for thumb2 later, since smaller patches are easy to review.

Also, two edge-case negative tests to exercise isSOImmTwoPartValSub are added.

Thanks for the test changes.

I have added tests for thumb2, however current patch does not apply to thumb2.

I'm a bit confused then why there are Thumb2 changes in FoldImmediate?

Thanks for the test changes.

I have added tests for thumb2, however current patch does not apply to thumb2.

I'm a bit confused then why there are Thumb2 changes in FoldImmediate?

I do not think there are Thumb2 changes in FoldImmediate. You can see NewUseOpc1 and NewUseOpc2 are identical
for all thumb2 related code. Actually the original NewUseOpc is enough for thumb2. So I should change it back (only) for thumb2 in FoldImmediate?

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

Sorry, I made a mistake, the changes to thumb are necessary, since

  • BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc), + BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc1), NewReg) .addReg(Reg1, getKillRegState(isKill)) .addImm(SOImmValV1) .add(predOps(ARMCC::AL)) .add(condCodeOp());
  • UseMI.setDesc(get(NewUseOpc)); + UseMI.setDesc(get(NewUseOpc2));

This piece of code is in the critical path, all cases will walk though it.

It is necessary to substitute NewUseOpc with NewUseOpc1 & NewUseOpc2.

I will upload a new patch later, with tests of orr and eor.

benshi001 added a comment.EditedAug 11 2020, 3:52 AM

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

Sorry, I made a mistake, the changes to thumb are necessary, since

\-  BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc),
\+  BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc1),
           NewReg)
       .addReg(Reg1, getKillRegState(isKill))
       .addImm(SOImmValV1)
       .add(predOps(ARMCC::AL))
       .add(condCodeOp());
\-  UseMI.setDesc(get(NewUseOpc));
\+  UseMI.setDesc(get(NewUseOpc2));

This piece of code is in the critical path, all cases will walk though it.

It is necessary to substitute NewUseOpc with NewUseOpc1 & NewUseOpc2.

I will upload a new patch later, with tests of orr and eor.

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

Sorry, I made a mistake, the changes to thumb are necessary, since

BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc),
         NewReg)
     .addReg(Reg1, getKillRegState(isKill))
     .addImm(SOImmValV1)
     .add(predOps(ARMCC::AL))
     .add(condCodeOp());
UseMI.setDesc(get(NewUseOpc));

is changed to

BuildMI(*UseMI.getParent(), UseMI, UseMI.getDebugLoc(), get(NewUseOpc1),
       NewReg)
   .addReg(Reg1, getKillRegState(isKill))
   .addImm(SOImmValV1)
   .add(predOps(ARMCC::AL))
   .add(condCodeOp());
UseMI.setDesc(get(NewUseOpc2));

This piece of code is in the critical path, all cases will walk though it.

It is necessary to substitute NewUseOpc with NewUseOpc1 & NewUseOpc2.

I will upload a new patch later, with tests of orr and eor.

benshi001 updated this revision to Diff 284667.Aug 11 2020, 4:58 AM

If the logic isn't needed, then yes, I'd recommend removing. Currently I'm seeing logic added for Thumb2 opcodes so I would expect some changes and testing and if that isn't case, then I guess the changes aren't needed. Looks like the ORR and EOR paths also need to be tested?

I have uploaded a new patch with tests of eor and orr.

The changes for Thumb2 are unavoidable due to the piece of code in the critical path.

benshi001 updated this revision to Diff 284956.Aug 11 2020, 7:56 PM

I have uploaded a new patch, in which more tests for orr and eor are added.

My patch also affect (improve) eor and orr, which I did not realize.

Currently, 12 positive test cases and 2 negative test cases are introduced, which should cover all corners.

I think the fact that you're having to make some Thumb2 changes to get around how the code is organised, suggests that the code should be reorganised instead. I'd suggest:

  • Pre-commit the two-part-imm.ll test.
  • Split the arm and thumb paths in an NFC patch, refactoring common code.
  • Then make your arm changes.
benshi001 updated this revision to Diff 285228.Aug 12 2020, 6:13 PM
benshi001 retitled this revision from [ARM] Optimize immediate selection to [ARM][test] Add more tests of two-part immediates.
benshi001 edited the summary of this revision. (Show Details)

I think the fact that you're having to make some Thumb2 changes to get around how the code is organised, suggests that the code should be reorganised instead. I'd suggest:

  • Pre-commit the two-part-imm.ll test.
  • Split the arm and thumb paths in an NFC patch, refactoring common code.
  • Then make your arm changes.

Thanks. This patch now only contains the improved test case two-part-imm.ll. And I will
submit the split of the arm and thumb paths in another NFC patch.

Really appreciate your suggestion.

benshi001 updated this revision to Diff 285553.Aug 13 2020, 9:32 PM

More tests are added for testing future patches.

samparker accepted this revision.Aug 14 2020, 12:13 AM

Thanks, but please feel free to commit tests without review.

This revision is now accepted and ready to land.Aug 14 2020, 12:13 AM
This revision was automatically updated to reflect the committed changes.