This is an archive of the discontinued LLVM Phabricator instance.

ARM: Introduce conservative load/store optimization mode
ClosedPublic

Authored by MatzeB on Feb 8 2016, 6:56 PM.

Details

Summary

Most of the time ARM has the CCR.UNALIGN_TRP bit set to false which
means that unaligned loads/stores do not trap and even extensive testing
will not catch these bugs. However the multi/double variants are not
affected by this bit and will still trap. In effect a more aggressive
load/store optimization will break existing (bad) code.

These bugs do not necessarily manifest in the broken code where the
misaligned pointer is formed but often later in perfectly legal code
where it is accessed. This means recompiling system libraries (which
have no alignment bugs) with a newer compiler will break existing
applications (with alignment bugs) that worked before.

So (under protest) I implemented this safe mode which limits the
formation of multi/double operations to cases that are not affected by
user code (stack operations like spills/reloads) or cases where the
normal operations trap anyway (floating point load/stores). It is
disabled by default.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 47286.Feb 8 2016, 6:56 PM
MatzeB retitled this revision from to ARM: Introduce conservative load/store optimization mode.
MatzeB updated this object.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
MatzeB updated this revision to Diff 47288.Feb 8 2016, 7:03 PM

Slightly improved comments.

rengolin added inline comments.Feb 11 2016, 3:57 AM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
67 ↗(On Diff #47288)

Maybe a name that has more to do with alignment?

Like "arm-unaligned-load-store", enabled by default?

test/CodeGen/ARM/ldrd.ll
19 ↗(On Diff #47288)

The preferred way is to omit the check-prefix and use CHECK for "ALL".

jmolloy accepted this revision.Feb 15 2016, 7:19 AM
jmolloy edited edge metadata.

This is a bit grim, but it looks generally OK. I'd prefer the name to be something more explanatory, like "-arm-cripple-load-store-optimizer" but I can see why you might not want to call it that.

James

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
929 ↗(On Diff #47288)

Should this return "true" if unaligned accesses are disabled?

This revision is now accepted and ready to land.Feb 15 2016, 7:19 AM
MatzeB updated this revision to Diff 48020.Feb 15 2016, 3:23 PM
MatzeB edited edge metadata.

Rename switch to -arm-assume-misaligned-load-store, incorporate feedback.

Thanks for the views.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
67 ↗(On Diff #47288)

What about arm-assume-misaligned-load-store?
I'd not enable this by default, in the end there is code with undefined behavior involved for this option to be necessary, such workarounds should not be defaults.

929 ↗(On Diff #47288)

I check at the place where I used the function. I'd like to think of this function as a check for a property on the program. A property of the program should be independent of the state of some cl::opt switch to I rather check that switch where the function is used.

ping, is this ok to commit Renato?

rengolin accepted this revision.Mar 2 2016, 10:56 AM
rengolin edited edge metadata.

Sorry, when James approved, I looked at the comments and they were all right, but didn't approve as well, as I thought unnecessary.

LGTM! :)

This revision was automatically updated to reflect the committed changes.