This is an archive of the discontinued LLVM Phabricator instance.

[ARM] generate armv6m eXecute Only (XO) code
ClosedPublic

Authored by stuij on Jun 13 2023, 3:03 AM.

Details

Summary

[ARM] generate armv6m eXecute Only (XO) code for immediates, globals

Previously eXecute Only (XO) support was implemented for targets that support
MOVW/MOVT (~armv7+). See: https://reviews.llvm.org/D27449

XO prevents the compiler from generating data accesses to code sections. This
patch implements XO codegen for armv6-M, which does not support MOVW/MOVT, and
must resort to the following general pattern to avoid loads:

movs    r3, :upper8_15:foo
lsls    r3, #8
adds    r3, :upper0_7:foo
lsls    r3, #8
adds    r3, :lower8_15:foo
lsls    r3, #8
adds    r3, :lower0_7:foo
ldr     r3, [r3]

This is equivalent to the code pattern generated by GCC.

The above relocations are new to LLVM and have been implemented in a parent
patch: https://reviews.llvm.org/D149443.

This patch limits itself to implementing codegen for this pattern and enabling
XO for armv6-M in the backend.

Separate patches will follow for:

  • switch tables
  • replacing specific loads from constant islands which are spread out over the ARM backend codebase. Amongst others: FastISel, call lowering, stack frames.

Diff Detail

Event Timeline

stuij created this revision.Jun 13 2023, 3:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 3:03 AM
stuij requested review of this revision.Jun 13 2023, 3:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 3:03 AM
stuij edited the summary of this revision. (Show Details)Jun 13 2023, 3:08 AM
stuij updated this revision to Diff 531801.Jun 15 2023, 9:44 AM
stuij edited the summary of this revision. (Show Details)

moved some code from ARMSubtarget.cpp from D149444 to here

stuij updated this revision to Diff 532181.Jun 16 2023, 8:55 AM

fix remaining known bug and clean up code

stuij retitled this revision from [ARM] generate eXecute Only code for immidiates, globals, externals to [ARM] generate armv6m eXecute Only (XO) code.Jun 16 2023, 9:00 AM
stuij edited the summary of this revision. (Show Details)
stuij added reviewers: john.brawn, simonwallis2.
john.brawn added inline comments.Jun 19 2023, 5:25 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
3962

This should be genExecuteOnly, as it's not thumb1-specific that we want to avoid generating a constant pool, it's just that up until now execute-only implied useMovt.

llvm/lib/Target/ARM/ARMInstrThumb.td
1613

This should be tMOVi32imm for consistency with everything else (all other 16-bit thumb instructions, or pseudoinstructions that expand to 16-bit thumb instructions, are named tSomething).

llvm/lib/Target/ARM/ARMPredicates.td
227–228

I don't think having a genT1ExecuteOnly predicate is a good idea, as it combines a bunch of things which makes things less clear when looking at the tablegen files. I think what we need is:

  • DontGenExecuteOnly which is just the negation of GenExecuteOnly
  • Patterns that are Requires<[GenT1ExecuteOnly]> are instead Requires<[IsThumb1Only, GenExecuteOnly, DontUseMovt]>
  • tLDRLIT_ga_abs should have Requires<[IsThumb, DontUseMovt, DontGenExecuteOnly]>
stuij updated this revision to Diff 532882.Jun 20 2023, 6:06 AM
stuij marked 2 inline comments as done.

address review comments

stuij added inline comments.Jun 20 2023, 6:19 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
3962

right, thanks! I'm guessing you meant just changing genT1ExecuteOnly to genExecuteOnly, as we still want to generate MOVW/MOVT for efficiency.

llvm/lib/Target/ARM/ARMInstrThumb.td
1613

Thanks. Yes, I had that initially, but then opted for t1, as it's just clearer. I do agree in hindsight that following convention is better here.

llvm/lib/Target/ARM/ARMPredicates.td
227–228

The reason why I decided to create GenT1ExecuteOnly, was actually because UseMovt itself is using a bunch of things which make sense for the specific use case of using MOVT, but it can't be used for just the negation of the MOVT instruction being there:

bool ARMSubtarget::useMovt() const {
  // NOTE Windows on ARM needs to use mov.w/mov.t pairs to materialise 32-bit
  // immediates as it is inherently position independent, and may be out of
  // range otherwise.
  return !NoMovt && hasV8MBaselineOps() &&
         (isTargetWindows() || !OptMinSize || genExecuteOnly());
}

It becomes confusing to use useMovt in for example this case where people will conflate useMovt with the MOVT instruction being there or not. And in this case, we're pulling things in that we might not. Things like isTargetWindows aren't relevant here and genXO is orthogonal in our use case.

I would myself prefer being clear about our intent: generating XO for T1, which is captured by GenT1ExecuteOnly. It's not obvious to someone looking at the code that the clauses which make up the decision for generating t1 code are related (I bet not everyone knows the check for armv8m.base is there because it is mostly T1, but does include movw/movt). Just looking at the predicate list doesn't make these details clear.

Did you mean to change llvm/test/tools/llvm-objcopy/ELF/strip-preserve-atime.test in this patch?

stuij added a comment.Jun 21 2023, 2:04 AM

Did you mean to change llvm/test/tools/llvm-objcopy/ELF/strip-preserve-atime.test in this patch?

No I did not! Will remove.

stuij updated this revision to Diff 533187.Jun 21 2023, 2:29 AM

remove strip-preserve-atime.test changes

john.brawn added inline comments.Jun 21 2023, 7:49 AM
llvm/lib/Target/ARM/ARMPredicates.td
227–228

Is see. In that case keeping GenT1ExecuteOnly is fine, but I think the logic should move into the predicate itself, i.e.

def GenT1ExecuteOnly : Predicate<"Subtarget->genExecuteOnly() && Subtarget->isThumb1Only() && !Subtarget->hasV8MBaselineOps()>"
229

This isn't needed as it isn't used anywhere.

stuij updated this revision to Diff 533526.Jun 22 2023, 3:18 AM
stuij marked an inline comment as done.

address review comments

stuij marked an inline comment as done.Jun 22 2023, 3:18 AM
stuij added inline comments.
llvm/lib/Target/ARM/ARMPredicates.td
227–228

done!

john.brawn accepted this revision.Jun 22 2023, 5:01 AM

LGTM, though please run clang-format on this patch before committing (pre-merge is showing a failure for this in ARMExpandPseudoInsts.cpp).

This revision is now accepted and ready to land.Jun 22 2023, 5:01 AM
This revision was landed with ongoing or failed builds.Jun 23 2023, 2:51 AM
This revision was automatically updated to reflect the committed changes.
stuij marked an inline comment as done.

@stuij This is breaking on EXPENSIVE_CHECKS builds - please can you take a look? https://lab.llvm.org/buildbot/#/builders/104/builds/12276

stuij added a comment.Jun 23 2023, 8:40 AM

@RKSimon: I've disabled the test for now while I figure out what's going on: https://reviews.llvm.org/rG5ddd561cb5e2