Page MenuHomePhabricator

[ARM] Generate CMSE instructions from CMSE intrinsics
ClosedPublic

Authored by chill on Nov 18 2019, 10:27 AM.

Details

Summary

This patch adds instruction selection patterns for the TT, TTT, TTA, and TTAT
instructions and tests for llvm.arm.cmse.tt, llvm.arm.cmse.ttt,
llvm.arm.cmse.tta, and llvm.arm.cmse.ttat intrinsics (added in a previous
patch).

Patch by Javed Absar.

Diff Detail

Event Timeline

chill created this revision.Nov 18 2019, 10:27 AM

Maybe I'm doing something wrong tried to apply these patches but when trying to build code which uses cmse I get

Cannot select: intrinsic %llvm.arm.cmse.tt
fatal error: error in backend: Cannot select: intrinsic %llvm.arm.cmse.tt
clang-10: error: clang frontend command failed with exit code 70 (use -v to see invocation)

@sigvartmh FYI, this patch enables selection of the LLVM CMSE intrinsics.

dmgreen accepted this revision.Nov 19 2019, 4:16 AM

LGTM, with a few nits about formatting.

llvm/lib/Target/ARM/ARMInstrThumb2.td
4554–4565

The Requires might be better on the next line.

llvm/test/CodeGen/ARM/intrinsics-cmse.ll
8

Remove the #0

64

Remove this line.

65

Remove the #1

This revision is now accepted and ready to land.Nov 19 2019, 4:16 AM
snidertm marked 2 inline comments as done.Nov 19 2019, 9:39 AM

Was the "error in backend" issue mentioned in the comment above resolved?

llvm/lib/Target/ARM/ARMInstrThumb2.td
4554–4565

Agree.

llvm/test/CodeGen/ARM/intrinsics-cmse.ll
3

Should we also add a RUN with -mcpu=cortex-m33?

dmgreen added inline comments.Nov 19 2019, 9:44 AM
llvm/test/CodeGen/ARM/intrinsics-cmse.ll
3

Oh, I hadn't noticed that. Good point

It may be better to also not explicitly use a cpu. It likely doesn't matter too much for this test, but for some of the other tests with more code using the architecture only will produce less scheduling artifacts. (I guess you could argue that both should be tested too, but that can be quite verbose).

chill planned changes to this revision.Nov 19 2019, 9:58 AM

Awesome I'll test it out with my use case and report back if it works :) and thank you so much for your work.

chill updated this revision to Diff 230297.Nov 20 2019, 11:12 AM
This revision is now accepted and ready to land.Nov 20 2019, 11:12 AM
chill marked 5 inline comments as done.Nov 20 2019, 11:12 AM
snidertm accepted this revision.Nov 20 2019, 1:36 PM
dmgreen accepted this revision.Nov 20 2019, 2:34 PM

Yep. Still LGTM.

I have just added a lot of little formatting suggestions. You are free to ignore any or all of them if you disagree. Whatever you think is best. No need for a re-review.

Oh, and @sigvartmh, I believe this is something like step 4 out of 8 or so. It will not yet do the register clearing that you would expect from a CMSE implementation, for example. That will be the interesting part.

llvm/lib/Target/ARM/ARMInstrThumb2.td
4533

I would make it look something like this, which I think is pretty standard for the arm backend. Maybe something like:

def t2TT : T2TT<0b00, "tt",
                [(set rGPR:$Rt, (int_arm_cmse_tt   GPRnopc:$Rn))]>,
           Requires<[IsThumb,Has8MSecExt]>;
llvm/test/CodeGen/ARM/intrinsics-cmse.ll
4

I don't think you actually need a datalayout for llc tests, so long as a triple is given.

6

These can be removed.

7

dso_local and local_unnamed_addr can be removed.

12

I would move this to just below the function definition, to place them where the update_llc_test_checks lines would go.

For something like this, is might make sense to use the script just to be sure about registers and whatnot.

sigvartmh added a comment.EditedNov 21 2019, 3:10 AM

@dmgreen That's fine by me. I do understand that this can take some time to add everything which is needed. As always thank you so much for working on this :)

Question: Is this sufficient to generate code which sets the processor into non-secure handler/thread mode? I'm fine with not having everything clean up all the registeres etc. I'm just going to use it for testing purposes at the moment to prepare a build system to handle llvm as a back-end for ARMv8-m.

chill added a comment.Nov 22 2019, 6:09 AM

Question: Is this sufficient to generate code which sets the processor into non-secure handler/thread mode? I'm fine with not having everything clean up all the registeres etc. I'm just going to use it for testing purposes at the moment to prepare a build system to handle llvm as a back-end for ARMv8-m.

No, this is not enough. In the incoming patches, the only mode-change relevant things would be CMSE entry functions returning via BXNS, and calling
non-secure functions via BLXNS, other than that, I would think setting the execution mode is out of the scope of the compiler.

The assembly support should be compete already, though.

chill updated this revision to Diff 230648.Nov 22 2019, 7:12 AM
chill marked 3 inline comments as done.
chill added a comment.Nov 22 2019, 7:14 AM

Will commit next week, since we're on the move and might not be able to promptly react to breakage.

This revision was automatically updated to reflect the committed changes.