[PATCH] D141776: [X86] `X86TargetLowering`: override `allowsMemoryAccess()`
Phoebe Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 18 20:13:18 PST 2023
pengfei added inline comments.
================
Comment at: llvm/lib/Target/X86/X86ISelLowering.h:1021-1026
+ bool allowsMemoryAccess(LLVMContext &Context, const DataLayout &DL, EVT VT,
+ const MachineMemOperand &MMO,
+ unsigned *Fast) const {
+ return allowsMemoryAccess(Context, DL, VT, MMO.getAddrSpace(),
+ MMO.getAlign(), MMO.getFlags(), Fast);
+ }
----------------
lebedev.ri wrote:
> RKSimon wrote:
> > lebedev.ri wrote:
> > > pengfei wrote:
> > > > Why do we need this and doing the same thing as `TargetLoweringBase`?
> > > Because otherwise we get a compilation failure.
> > it looks like not all the TargetLoweringBase::allowsMemoryAccess variants are marked as virtual - that probably needs cleaning up?
> > that probably needs cleaning up?
> ABSOLUTELY NOT!
> There is only a single true `allowsMemoryAccess()`, all others are just helpers
> to call it with different types of arguments. If they are all virtual,
> and not just one of them, then you will essentially need to override them all.
> It would not be an improvement.
I don't get the point. Why can't we use the helper directly but define a new one here? What are these helper used for if we need to define a same one each time?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141776/new/
https://reviews.llvm.org/D141776
More information about the llvm-commits
mailing list