[PATCH] D70407: [ARM] Generate CMSE instructions from CMSE intrinsics

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.

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.



================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:4533
 let hasSideEffects = 1 in
 def t2SG : T2I<(outs), (ins), NoItinerary, "sg", "", []>,
            Requires<[Has8MSecExt]> {
----------------
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]>;
```


================
Comment at: llvm/test/CodeGen/ARM/intrinsics-cmse.ll:3
+; RUN: llc < %s -mtriple=thumbebv8m.base | FileCheck %s
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+
----------------
I don't think you actually need a datalayout for llc tests, so long as a triple is given.


================
Comment at: llvm/test/CodeGen/ARM/intrinsics-cmse.ll:5
+
+; Function Attrs: nounwind readnone
+define dso_local i32 @test_tt(i8* readnone %p) local_unnamed_addr #0 {
----------------
These can be removed.


================
Comment at: llvm/test/CodeGen/ARM/intrinsics-cmse.ll:6
+; Function Attrs: nounwind readnone
+define dso_local i32 @test_tt(i8* readnone %p) local_unnamed_addr #0 {
+entry:
----------------
dso_local and local_unnamed_addr can be removed.


================
Comment at: llvm/test/CodeGen/ARM/intrinsics-cmse.ll:11
+}
+; CHECK-LABEL: test_tt:
+; CHECK: tt r{{[0-9]+}}, r{{[0-9]+}}
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70407/new/

https://reviews.llvm.org/D70407