This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Don't convert switches to lookup tables of pointers with ROPI/RWPI
ClosedPublic

Authored by olista01 on Sep 12 2016, 9:14 AM.

Details

Summary

With the ROPI and RWPI relocation models we can't always have pointers
to global data or functions in constant data, so don't try to convert
switches into lookup tables if the type of the PHI is a pointer type. We
can still safely emit lookup tables of any non-pointer type.

Diff Detail

Repository
rL LLVM

Event Timeline

olista01 updated this revision to Diff 71018.Sep 12 2016, 9:14 AM
olista01 retitled this revision from to [ARM] Don't convert switches to lookup tables of pointers with ROPI/RWPI.
olista01 updated this object.
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: llvm-commits.
efriedma requested changes to this revision.Sep 19 2016, 4:42 PM
efriedma added a reviewer: efriedma.
efriedma added a subscriber: efriedma.

Checking the type doesn't make sense; consider what happens if you have a PHI node with an operand i32 ptrtoint (i32* @g).

This revision now requires changes to proceed.Sep 19 2016, 4:42 PM
olista01 updated this revision to Diff 71912.Sep 20 2016, 3:38 AM
olista01 edited edge metadata.

Check whether the value requires relocation, rather than the type.

efriedma added inline comments.Sep 21 2016, 4:16 PM
lib/Transforms/Utils/SimplifyCFG.cpp
5151 ↗(On Diff #71912)

Should we be calling ValidLookupTableConstant here instead of calling shouldBuildLookupTablesForConstant directly?

hans added inline comments.Sep 21 2016, 4:29 PM
lib/Transforms/Utils/SimplifyCFG.cpp
4435 ↗(On Diff #71912)

It would seem more natural to do all the target-independent checks first, and then defer to TTI for the rest.

5151 ↗(On Diff #71912)

Isn't this check now already done in GetCaseResults?

olista01 updated this revision to Diff 72144.Sep 22 2016, 1:56 AM
olista01 removed rL LLVM as the repository for this revision.
olista01 marked 3 inline comments as done.
  • Remove old check of shouldBuildLookupTablesForConstant
  • Re-order ValidLookupTableConstant to do target-dependent checks last
joerg added a subscriber: joerg.Sep 22 2016, 6:41 AM

This change doesn't make sense to me. At most, ROPI/RWPI should enforce creation of PIC jump tables. There are some issues with the way we currently generate those, which is biting us on PPC64, but it is still the correct way forward.

The problem here is that this pass can create new global variables, which the backend can't recognise as jump tables (they may not actually be jump tables, as they could be tables of pointers to data).

There are a few different ways that ROPI/RWPI can be made to support pointers to globals in source code, with varying requirements on the rest of the compiler:

  1. Don't do anything special, the user will have to modify their code to avoid global variables initialised to the address of a global or function. This is currently done by clang, and requires that global pointers are not introduced by LLVM.
  2. Add dynamic initialisers in the frontend, called from the .init_array section. This is currently done by armclang (our clang-based bare-metal toolchain). We chose this option because it allows moving constants to a read-write section if they need dynamic initialisation, and emitting diagnostics for some C constructs that may not work properly with ROPI/RWPI. This also requires that global pointers are not introduced by LLVM.
  3. Use a small dynamic loader to fix-up any global pointers. This requires linker and runtime library support, I'm not aware of any implementations of this. This wouldn't be able to fix up constants when they are loaded into ROM. This would allow the compiler to introduce global pointers, but would require them to be in a read-write section, so that their value can be adjusted by the loader.

This change supports the first two, the other option would be to modify this pass to emit all lookup tables containing pointers as variables rather than constants (and possibly modify globalopt to avoid undoing that transformation). I prefer turning off this optimisation for pointers in ROPI/RWPI modes, as it avoids embedding too much knowledge about a rather unusual ARM-specific ABI in the midend.

efriedma added inline comments.Oct 5 2016, 5:25 PM
lib/Transforms/Utils/SimplifyCFG.cpp
4441 ↗(On Diff #72144)

I think this needs to be something more like:

if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
  if (!CE->isGEPWithNoNotionalOverIndexing())
    return false;
  if (!ValidLookupTableConstant(CE->getOperand(0)))
    return false;
}

Specifically, you need to recurse over GEPs to make sure they aren't doing anything that can't be represented in a global variable. (I guess it's an existing issue, but worth solving while you're here.)

olista01 added inline comments.Oct 6 2016, 9:36 AM
lib/Transforms/Utils/SimplifyCFG.cpp
4441 ↗(On Diff #72144)

Good catch, I'll put up a separate patch for that once I can find a good test for it.

efriedma accepted this revision.Oct 6 2016, 9:54 AM
efriedma edited edge metadata.

Okay, doing that in a separate patch is fine.

Otherwise LGTM.

This revision is now accepted and ready to land.Oct 6 2016, 9:54 AM
This revision was automatically updated to reflect the committed changes.