[PATCH] D114506: [AArch64][SVE]Support for SVE instrinsic in AArch64TargetTransformInfo

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

My understand of how `MemIntrinsicInfo` is used is not great so please forgive some potentially naive questions.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2070
+    Info.WriteMem = false;
+    Info.PtrVal = Inst->getArgOperand(1);
+    break;
----------------
How literal is the description of this property in `TargetTransformInfo.h`.  Specifically it says:
```
  /// If this is non-null, then analysis/optimization passes can assume that
  /// this intrinsic is functionally equivalent to a load/store from this
  /// pointer.
```
However, these are predicated loads and stores and thus they take an extra operand that controls when/if memory is accessed.

Looking at EarlyCSE I can see there exists support for the common masked_load and masked_store intrinsics, where masks are considered (see isNonTargetIntrinsicMatch), but I cannot see such for target intrinsics.

This makes me believe we cannot just populate `MemIntrinsicInfo` for the SVE intrinsics because the data will be incomplete and potentially cause things like EarlyCSE to do the wrong thing.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2097-2104
+  case Intrinsic::aarch64_sve_ld1:
+  case Intrinsic::aarch64_sve_st1:
+  case Intrinsic::aarch64_sve_ld2:
+  case Intrinsic::aarch64_sve_st2:
+  case Intrinsic::aarch64_sve_ld3:
+  case Intrinsic::aarch64_sve_st3:
+  case Intrinsic::aarch64_sve_ld4:
----------------
Is it safe to not set `MatchingId` here? My assumption is that means all SVE load/strore intrinsics will have the same Id and thus code like in EarlyCSE.cpp that compares the `MatchingId` of two instructions may not get whatever guarantee they're trying to obtain.




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

https://reviews.llvm.org/D114506