[PATCH] D147363: [AMDGPU] Add target hook to isGlobalMemoryObject

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/ScheduleDAGInstrs.cpp:537
 
-/// Returns true if MI is an instruction we are unable to reason about
-/// (like a call or something with unmodeled side effects).
-static inline bool isGlobalMemoryObject(MachineInstr *MI) {
+bool ScheduleDAGInstrs::isGlobalMemoryObject(MachineInstr *MI) {
   return MI->isCall() || MI->hasUnmodeledSideEffects() ||
----------------
kerbowa wrote:
> arsenm wrote:
> > I don't understand why you need to special case these. Isn't this more or less the same as removing hasSideEffects from the instruction? Could you achieve the same thing by using a pseudo memory operand that doesn't alias anything the instruction reads and writes from?
> Should the intrinsic have sideeffects in that case? We would need some special handling in isel if there is a mismatch between hasSideEffects between the intrinsics and the MachineInstrs.
HasSideEffects is a catch all for unmodeled properties. Selectively ignoring it implies there should be a different property to express this constraint. Given that side effects aren’t really enough to prevent pure arithmetic code motion in the original IR, it seems to me it shouldn’t have side effects


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147363