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

Fri Sep 1 00:11:21 PDT 2023

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();
   }
----------------
maksfb wrote:
> yota9 wrote:
> > 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!
> The current implementation captures the original intent, i.e. if the instruction modifies the value in memory it should pass both `isLoad()` and `isStore()` conditions. However, when we consider predicated instructions, it is not possible to statically tell if the instruction actually loads/stores. I.e. `mayLoad`/`mayStore` is a better naming. Please feel free to change.
+1 , I agree that "may" semantics looks much better in this situation :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159266