[PATCH] D153050: [mlir][ArmSME] Insert intrinsics to enable/disable ZA

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 2 inline comments as done.
c-rhodes added inline comments.


================
Comment at: mlir/lib/Dialect/ArmSME/Transforms/LegalizeForLLVMExport.cpp:57-58
+
+  // Enable ZA if the function has the 'arm_za' attribute and it hasn't already
+  // been enabled.
+  target.addDynamicallyLegalOp<func::FuncOp>([&](func::FuncOp funcOp) {
----------------
awarzynski wrote:
> This comment suggests that `target.addDynamicallyLegalOp()` would enable `ZA`, but that's not quite true, is it? Instead, it marks `funcOp` as dynamically legal and provides a custom legality check:
> * if `arm_za` attribute is present then legal if the first function Op is `arm_sme::aarch64_sme_za_enable`, 
> * if `arm_za` is not present, mark as legal.
> 
> I guess that the comment refers to the overall legalisation process rather then specifically to this hook?
> This comment suggests that `target.addDynamicallyLegalOp()` would enable `ZA`, but that's not quite true, is it? Instead, it marks `funcOp` as dynamically legal and provides a custom legality check:
> * if `arm_za` attribute is present then legal if the first function Op is `arm_sme::aarch64_sme_za_enable`, 
> * if `arm_za` is not present, mark as legal.
> 
> I guess that the comment refers to the overall legalisation process rather then specifically to this hook?

Good point I've updated the comment hopefully makes more sense, cheers


================
Comment at: mlir/test/Dialect/ArmSME/enable-arm-za.mlir:1
+// RUN: mlir-opt %s -enable-arm-streaming=enable-za -convert-vector-to-llvm="enable-arm-sme" | FileCheck %s
+
----------------
awarzynski wrote:
> Could you add another RUN line without `enable-za` and verify that there's no `asm_sme.intr.za.{enable|disable}`?
> 
> It would also be good to test without the `-enable-arm-streaming` pass.
> Could you add another RUN line without `enable-za` and verify that there's no `asm_sme.intr.za.{enable|disable}`?

Done

> 
> It would also be good to test without the `-enable-arm-streaming` pass.

I misunderstood what you mean here somehow, will update again to add this 😄 


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

https://reviews.llvm.org/D153050