[PATCH] D64416: [AArch64] Add support for Transactional Memory Extension (TME)

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard requested changes to this revision.
ostannard added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7223
 
+  /* Transactional Memory Extentions (TME) intrinsics. Check range of __tcancel
+     argument, other intrinsics are handled automatically. */
----------------
This error checking should be done in Sema::CheckBuiltinFunctionCall, not CodeGen. 


================
Comment at: clang/test/CodeGen/aarch64-tme-errors.c:1
+// RUN: not %clang_cc1 -triple aarch64-eabi -fsyntax-only %s -o - 2>&1 | FileCheck %s
+
----------------
Tests for error messages should use clang's `-verify` option, and be in clang/test/Sema.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:713
+def int_aarch64_tcancel : Intrinsic<[], [llvm_i64_ty],
+                                    [ImmArg<0>, IntrNoMem, IntrHasSideEffects, IntrNoReturn, Throws]>;
+
----------------
Why is this IR intrinsic marked as `Throws`, but the C intrinsic is nothrows (`n`)?


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:715
+
+def int_aarch64_ttest   : GCCBuiltin<"__builtin_arm_ttest">,
+                          Intrinsic<[llvm_i64_ty], [], [IntrNoMem]>;
----------------
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?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1064
+      Sched<[WriteSys]> {
+  let Inst{31-0} = 0b11010101000000110011000001111111;
+  let DecoderMethod = "";
----------------
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.


================
Comment at: llvm/test/CodeGen/AArch64/tme.ll:29
+; Function Attrs: nounwind
+define dso_local i1 @cas(i32* nocapture %p, i32 %old, i32 %new) local_unnamed_addr #0 {
+entry:
----------------
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).


================
Comment at: llvm/test/CodeGen/AArch64/tme.ll:79
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon,+tme" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind }
----------------
Most of the attributes and metadata can be removed to simplify the test.


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

https://reviews.llvm.org/D64416