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

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 8 inline comments as done.
chill added inline comments.


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


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




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

https://reviews.llvm.org/D64416