This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Allow codegen for Armv6m eXecute-Only (XO) sections
ClosedPublic

Authored by stuij on Apr 28 2023, 6:38 AM.

Details

Summary

This patch moves the overall lower-bound arch restriction for Arm XO sections
from v8m to v6m. Actual implementation of code-gen for v6m will follow in
follow-up patches, which will include an implementation of relocations needed to
support this.

Diff Detail

Event Timeline

stuij created this revision.Apr 28 2023, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 6:38 AM
stuij requested review of this revision.Apr 28 2023, 6:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 28 2023, 6:38 AM
stuij edited the summary of this revision. (Show Details)Apr 28 2023, 6:41 AM
stuij added subscribers: john.brawn, simonwallis2.
stuij edited the summary of this revision. (Show Details)Apr 28 2023, 6:45 AM
simonwallis2 added inline comments.May 2 2023, 6:48 AM
llvm/lib/Target/ARM/ARMSubtarget.cpp
434 ↗(On Diff #517898)

V6M does not have Movt.
At face value, this line looks wrong and leads to about 30 unit test fails.

simonwallis2 added inline comments.May 2 2023, 7:17 AM
llvm/lib/Target/ARM/ARMSubtarget.cpp
434 ↗(On Diff #517898)

I clarify: there are no test fails with this patch on its own.
The unit test fails I saw where when building this patch in conjunction with related patch https://reviews.llvm.org/D149443

stuij added inline comments.May 23 2023, 3:54 AM
llvm/lib/Target/ARM/ARMSubtarget.cpp
434 ↗(On Diff #517898)

Correct, this shouldn't be here. Initially it looked like we were going to repurpose movt for v6m, but that should have been part of another patch. And now it looks like we'll handle v6m immediates separately. In any case I'll remove this.

stuij updated this revision to Diff 524755.May 23 2023, 9:10 AM

addressed review comments and updated tests

stuij edited the summary of this revision. (Show Details)May 23 2023, 9:11 AM
stuij marked 2 inline comments as done.
tschuett added inline comments.
llvm/lib/Target/ARM/ARMSubtarget.cpp
194 ↗(On Diff #524755)

What happens in release mode? At the top you now claim that ARMV6M is supported. Could hasV6MOps() silently return false?

stuij updated this revision to Diff 525079.May 24 2023, 2:38 AM

addressed review comment

llvm/lib/Target/ARM/ARMSubtarget.cpp
194 ↗(On Diff #524755)

This should really be caught by the frontend, but yes, I'm not sure why we use assert so often in this kind of code instead of unreachable. Changed.

stuij marked an inline comment as done.May 24 2023, 2:39 AM
simonwallis2 accepted this revision.May 31 2023, 5:12 AM
This revision is now accepted and ready to land.May 31 2023, 5:12 AM
MaskRay added inline comments.May 31 2023, 7:24 AM
clang/test/Driver/arm-execute-only.c
1–4

test/Driver tests should test just the Driver part, not sema/codegen, so we generally use -### to skip compilation.

-target has been deprecated since 3.x. Use --target=

MaskRay added inline comments.May 31 2023, 7:25 AM
llvm/lib/Target/ARM/ARMSubtarget.cpp
195 ↗(On Diff #525079)

If this code path is actually reachable, a proper error reporting will be better. At the least report_fatal_error can be used. Don't use llvm_unreachable for known-reachable code paths.

stuij updated this revision to Diff 530505.Jun 12 2023, 7:38 AM
stuij marked 2 inline comments as done.

addressed review comments

MaskRay accepted this revision.Jun 12 2023, 1:54 PM

Actual implementation of code-gen for v6m will follow in follow-up patches, which will include an implementation of relocations needed to support this.

Normally we implement the feature in llvm/ first, and the clang/lib/Driver change should be the last patch.
Some users just check whether clang --target=xxx accepts a given option, not test that the feature actually works (it's sometimes non-trivial to figure out all corners as they are not compiler feature implementers.)

clang/test/Driver/arm-execute-only.c
1–4

If you don't check -cc1 options with FileCheck, you can also replace -### with -fdriver-only -Werror and drop two -NOT: patterns below. You can play with some examples to see its effect:)

If the clang -c -fdriver-only -Werror command gives no output, you can use ... 2>&1 | count 0

stuij added a comment.Jun 13 2023, 3:23 AM

Normally we implement the feature in llvm/ first, and the clang/lib/Driver change should be the last patch.

Yes good point. I won't commit it yet. For this feature we're using Phab as a vehicle to swap patches, so we're reviewing these upstream as they become available.

clang/test/Driver/arm-execute-only.c
1–4

Very useful info. Thanks for the tips!

stuij updated this revision to Diff 531787.Jun 15 2023, 9:11 AM
  • Moved the backend parts of this patch to D149443.
  • Slightly tweaked the way we call the specific arch in the tests.
stuij added a comment.Jun 15 2023, 9:40 AM
  • Moved the backend parts of this patch to D149443.

Wrong Phab review -^ It should be D152795.

MaskRay accepted this revision.Jun 15 2023, 1:05 PM

Looks great!

clang/test/Driver/arm-execute-only.c
1–4

LGTM once the simplification is done (e.g. clang -c -fdriver-only -Werror)

stuij updated this revision to Diff 532684.Jun 19 2023, 9:01 AM

address review comments

stuij marked an inline comment as done.Jun 19 2023, 9:03 AM
clang/test/Driver/arm-execute-only.c
1–4

yup, thanks for that. I was struggling to get rid of those NOTs.

MaskRay accepted this revision.Jun 20 2023, 5:05 PM
This revision was landed with ongoing or failed builds.Aug 9 2023, 6:16 AM
This revision was automatically updated to reflect the committed changes.
stuij marked an inline comment as done.