Page MenuHomePhabricator

[ARM, FIX] ARMTargetLowering::isLegalAddressingMode can accept illegal addressing modes for Thumb1 target
ClosedPublic

Authored by eastig on Aug 8 2017, 7:07 AM.

Details

Summary

Investigating Corext-M23/M0+ performance regressions caused by https://reviews.llvm.org/D34583 "[LSR] Narrow search space by filtering non-optimal formulae with the same ScaledReg and Scale." it was found that ARMTargetLowering::isLegalAddressingMode can accept illegal addressing modes for the Thumb1 target (See https://bugs.llvm.org/show_bug.cgi?id=34106 for details). Thumb1 addressing modes do not support scaling values: positive greater than 1; any negative. Such addressing modes are illegal for the Thumb1 target. This bug causes LSR to insert additional IR operations which are lowered to more instructions than it's actually needed.

The patch fixes the issue.

Testing shows the regressions are fixed. We even get additional 6% improvements on the benchmarks. We also get improvements on code size (-Oz) because less instructions are generated.

Diff Detail

Repository
rL LLVM

Event Timeline

eastig created this revision.Aug 8 2017, 7:07 AM
efriedma added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
12400 ↗(On Diff #110187)

Maybe ((unsigned)AM.HasBaseReg + Scale) <= 2, like we use in other places? It doesn't come up often, but it's easy to support.

zzheng added a subscriber: zzheng.Aug 8 2017, 11:32 AM
eastig added inline comments.Aug 9 2017, 4:34 AM
lib/Target/ARM/ARMISelLowering.cpp
12400 ↗(On Diff #110187)

This will accept negative scales: (unsigned)true + (unsigned)-1 == 0
Thumb1 addressing modes do not support negative scales. I'll put this in the comment.

eastig updated this revision to Diff 110360.Aug 9 2017, 4:44 AM
eastig edited the summary of this revision. (Show Details)

Added comments that negative scales are not supported in Thumb1.

efriedma added inline comments.Aug 9 2017, 12:26 PM
lib/Target/ARM/ARMISelLowering.cpp
12400 ↗(On Diff #110187)

Oh, err, wow, that's a really nasty use of wrapping arithmetic. I'll propose a patch to clarify it.

We should still be able to support the case where "AM.Scale == 2" and "HasBaseReg" is false for Thumb1.

eastig added inline comments.Aug 9 2017, 12:49 PM
lib/Target/ARM/ARMISelLowering.cpp
12400 ↗(On Diff #110187)

What a pass creates this situation: "AM.Scale == 2" and "HasBaseReg == false"? How to write an IR test for it?

$ echo "int a(int x) { return *(int*)(x*2); }" | clang -x c - -S -O2 -mllvm -debug-only=codegenprepare --target=armv7--eabihf -o /dev/null
CGP: Found      local addrmode: [2*%x]
eastig added a comment.Aug 9 2017, 1:16 PM
$ echo "int a(int x) { return *(int*)(x*2); }" | clang -x c - -S -O2 -mllvm -debug-only=codegenprepare --target=armv7--eabihf -o /dev/null
CGP: Found      local addrmode: [2*%x]

Thank you for the test case. And it's a nice demonstration of the pipeline technique. I'll use it to generate more test cases.

eastig updated this revision to Diff 111960.Aug 21 2017, 5:52 AM
  • Moved Thumb1 related code into a separate function: ARMTargetLowering::isLegalT1ScaledAddressingMode
  • Added support of the case: Scale == 2 and there is no base register
This revision is now accepted and ready to land.Aug 23 2017, 1:05 PM
This revision was automatically updated to reflect the committed changes.

Thank you, Eli.