This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add support for Transactional Memory Extension (TME)
ClosedPublic

Authored by chill on Jul 9 2019, 6:36 AM.

Details

Summary
  TME is a future architecture technology, documented in
  
  https://developer.arm.com/architectures/cpu-architecture/a-profile/exploration-tools
  https://developer.arm.com/docs/ddi0601/a
  
  More about the future architectures:
  
  https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/new-technologies-for-the-arm-a-profile-architecture
  
  This patch adds support for the TME instructions TSTART, TTEST, TCOMMIT, and
  TCANCEL and the target feature/arch extension "tme".
  
  It also implements TME builtin functions, defined in ACLE Q2 2019
  (https://developer.arm.com/docs/101028/latest)
  
Patch by Javed Absar and Momchil Velikov

Diff Detail

Event Timeline

chill created this revision.Jul 9 2019, 6:36 AM
ostannard requested changes to this revision.Jul 10 2019, 2:12 AM
ostannard added a subscriber: ostannard.
ostannard added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
7223 ↗(On Diff #208668)

This error checking should be done in Sema::CheckBuiltinFunctionCall, not CodeGen.

clang/test/CodeGen/aarch64-tme-errors.c
1 ↗(On Diff #208668)

Tests for error messages should use clang's -verify option, and be in clang/test/Sema.

llvm/include/llvm/IR/IntrinsicsAArch64.td
745

Why is this IR intrinsic marked as Throws, but the C intrinsic is nothrows (n)?

747

Are you sure it's safe for this to be IntrNoMem and not IntrHasSideEffects? I think this means that all calls to this intrinsic can be folded together, even across transaction boundaries.

More generally, I wonder if it would be safer to model all of these intrinsics as reading and writing memory, because we don't want them to be re-ordered with respect to any other memory accesses (though maybe IntrHasSideEffects is enough for that). Do we have any documentation on how these intrinsics change (if at all) the C or LLVM rules regarding data races?

llvm/lib/Target/AArch64/AArch64InstrFormats.td
1028

This isn't how we use instruction format classes. There should be one class here for each group with the same operands and encoding. Some of these might be able to re-use existing format classes if they fit into existing parts of the encoding space.

llvm/test/CodeGen/AArch64/tme.ll
30

It would be better to test each intrinsic in a separate function, so that this won't be affected by irrelevant codegen changes (e.g. basic block layout).

80

Most of the attributes and metadata can be removed to simplify the test.

This revision now requires changes to proceed.Jul 10 2019, 2:12 AM
chill updated this revision to Diff 209188.Jul 11 2019, 5:50 AM
chill edited the summary of this revision. (Show Details)

Thanks for the review. I've addressed the comments.

chill marked 8 inline comments as done.Jul 11 2019, 6:09 AM
chill added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
745

I've now marked both as nothrow.

747

It's indeed probably unsafe. I've changed ttest now to have side effects, so it's on the chain with tstart/tcommit/tcancel.

I would like to impose the minimun set of restrictions regarding this set of intrinsics and associated instructions. There isn't yet a specification how this extension fits in the AArch64 or C/C++ memory model. Intuitively, regarding reordering of memory accesses, I would expect tstart to have Acquire semantics and tcommit - Release; but then that's speculation at this point, moreover I couldn't find a way to express that.

So, now

  • tstart and tcommit both read/write memory and have side efects;
  • ttest has side-effects (so it's not reordered with tstart and tcommit, but otherwise everythign is free to move around it.
  • tcancel is the only one left with IntrNoMem and IntrHasSideEffects - it's IntrNoReturn so every operation following it is dead; if such an operation was to be reordered with tcancel it does not become less dead as tcancel discards its effects. Same for reordering in the opposite direction.
chill marked 2 inline comments as done.Jul 11 2019, 6:10 AM
ostannard accepted this revision.Jul 11 2019, 6:18 AM

LGTM, thanks.

This revision is now accepted and ready to land.Jul 11 2019, 6:18 AM
This revision was automatically updated to reflect the committed changes.
chill reopened this revision.Jul 18 2019, 1:44 AM

I reverted the patch, have to rework tcancel.

This revision is now accepted and ready to land.Jul 18 2019, 1:44 AM
chill planned changes to this revision.Jul 18 2019, 1:45 AM
chill updated this revision to Diff 210768.Jul 19 2019, 1:38 AM

Changed tcancel implementation.

This revision is now accepted and ready to land.Jul 19 2019, 1:38 AM
chill requested review of this revision.Jul 19 2019, 1:39 AM
This revision is now accepted and ready to land.Jul 19 2019, 2:56 AM
t.p.northover added inline comments.Jul 19 2019, 5:05 AM
clang/test/Sema/aarch64-tme-errors.c
2

I don't think the Sema checks need to be split over so many files. One for the whole of transactional seems enough. Similarly for CodeGen really.

llvm/test/CodeGen/AArch64/tme-tcancel.ll
6 ↗(On Diff #210768)

Testing more values than 0 would be a good idea.

chill planned changes to this revision.Jul 19 2019, 6:08 AM
RKSimon added inline comments.Jul 19 2019, 6:48 AM
llvm/test/CodeGen/AArch64/tme-tcancel.ll
1 ↗(On Diff #210768)

Would it make sense to add -verify-machineinstrs to all these Codegen/AArch64/tme-*.ll tests?

chill updated this revision to Diff 211140.Jul 22 2019, 10:19 AM
This revision is now accepted and ready to land.Jul 22 2019, 10:19 AM
chill requested review of this revision.Jul 22 2019, 10:21 AM
chill marked 3 inline comments as done.
chill added a comment.Jul 29 2019, 2:33 AM

If there are no objections, I'll go ahead with committing that Soon(tm) on the basis of previous acceptance.

chill accepted this revision.Jul 30 2019, 4:02 AM
This revision is now accepted and ready to land.Jul 30 2019, 4:02 AM
chill updated this revision to Diff 212332.Jul 30 2019, 6:41 AM

Rebase on top of master.

This revision was automatically updated to reflect the committed changes.