[PATCH] D58635: AMDGPU: Remove IntrReadMem from memtime/memrealtime intrinsics

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked 2 inline comments as done.
arsenm added a comment.

In D58635#1409379 <https://reviews.llvm.org/D58635#1409379>, @rampitec wrote:

> 1. These intrinsics read internal state. If not IntrReadMem something needs to be added to model it.


IntrReadMem doesn't mean it reads memory, it means it only reads memory. Reading and writing memory is the default assumption. The negated behavior of the intrinsic memory attributes is part of why the tablegen inference is so broken

> 2. Patch does more than stated in the description with no justification.





================
Comment at: lib/Target/AMDGPU/SMInstructions.td:160
+
+  // FIXME: This should be definitively mayStore = 0.
+  let mayStore = ?;
----------------
rampitec wrote:
> It was zero before this patch.
This is to defeat tablegen's completely broken inference of instruction flags based on intrinsic properties. This conflicts with the assumed mayStore since the intrinsic isn't IntrReadMem. The assumption the flags directly correspond to the intrinsic attributes is broken because the weird memory dependency is tracked here with hasSideEffects. It's even more broken because this was working with readcyclecounter already, since it isn't a target intrinsic.


================
Comment at: lib/Target/AMDGPU/SMInstructions.td:797
 
+def : GCNPat <
+  (i64 (int_amdgcn_s_memrealtime)),
----------------
rampitec wrote:
> It does not belong to the patch.
This was part of trying to avoid not explicitly setting the mayStore flag, but that ultimately didn't work


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

https://reviews.llvm.org/D58635