[PATCH] D159266: [BOLT] Provide generic implementations for isLoad/isStore
Job Noorman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list