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

Fri Sep 1 00:37:27 PDT 2023

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:
> 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 :)
Agreed, committed the renaming separately in eafe4ee2e8447d7daf4aaeabcfaf8f8854dd9575.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159266