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

Maksim Panchenko via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 31 10:34:04 PDT 2023

maksfb accepted this revision.
maksfb added inline comments.
This revision is now accepted and ready to land.

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:
> 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.

Comment at: bolt/test/RISCV/load-store.s:3
+// RUN: link_fdata --no-lbr %s %t %t.fdata
+// RUN: llvm-bolt %t -o /dev/null -data=%t.fdata -dyno-stats | FileCheck %s

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list