Page MenuHomePhabricator

[ARM] Temporarily disable globals promotion to constant pools to prevent miscompilation
ClosedPublic

Authored by iid_iunknown on May 23 2017, 9:11 AM.

Details

Summary

A temporary workaround for PR32780 - rematerialized instructions accessing the same promoted global through different constant pool entries.

The patch turns off the globals promotion optimization leaving all its code in place, so that it can be easily turned on once PR32780 is fixed.

Since this is a miscompilation issue causing generation of misbehaving code, and the problem is very subtle, the patch might be valuable enough to get into 4.0.1.

Diff Detail

Repository
rL LLVM

Event Timeline

iid_iunknown created this revision.May 23 2017, 9:11 AM
efriedma edited edge metadata.May 23 2017, 10:36 AM

I would suggest switching the option (arm-promote-constant) to default to false rather than commenting out the code, so we don't have to XFAIL the unit-tests.

At a higher level, this seems okay.

I would suggest switching the option (arm-promote-constant) to default to false rather than commenting out the code, so we don't have to XFAIL the unit-tests.

Thanks for your comment, Eli.

Yes, initially I thought about option adjusting too as it's easier to do, but refused this idea because one might hit the unexpected miscompilation if setting the option to true explicitly in the command line. One will get no warning about possible issues with this option.

Do you confirm that such behavior is acceptable?

Do you confirm that such behavior is acceptable?

Yes, this is fine; we switch off optimizations which aren't ready to use with options all the time.

jmolloy edited edge metadata.May 23 2017, 11:19 AM

+1 from me.

Addressing the review comments.

Updated the patch.

Extra corrections:

  • added a FIXME comment to revert the option to true when the problem is resolved;
  • added explicit -arm-promote-constant to the tests as they expect promotion to happen.
This revision is now accepted and ready to land.May 23 2017, 12:14 PM
iid_iunknown closed this revision.May 23 2017, 12:38 PM

Eli, James, thanks for the review!