[PATCH] D159266: [BOLT] Provide generic implementations for isLoad/isStore

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: bolt/include/bolt/Core/MCPlusBuilder.h:617
   virtual bool isLoad(const MCInst &Inst) const {
-    llvm_unreachable("not implemented");
-    return false;
+    return Info->get(Inst.getOpcode()).mayLoad();
   }
----------------
yota9 wrote:
> I'm not completely sure, but shouldn't we check here that we may not store here? E.g. atomic instructions seems would return both true for store and load and the current logic is designed here to handle load-only instructions. What do you think?
Good question. As far as I can tell, this is only used for dyno stats so it should be fine to count these atomics as both loads and stores? This would already happen for the [X86 target](https://github.com/llvm/llvm-project/blob/34a35a8b244243f5a4ad5d531007bccfeaa0b02e/bolt/lib/Target/X86/X86MCPlusBuilder.cpp#L363C27-L363C27).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159266