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

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 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();
   }
----------------
jobnoorman wrote:
> 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).
Probably you're right. Also since internal implementations used inside MIB would still do what they intended to do, probably for now it is not a problem. Although I'm not completely sure do we need this extra check for the future or not :) Probably meta team would decide, but in other aspects LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159266