[PATCH] D141776: [X86] `X86TargetLowering`: override `allowsMemoryAccess()`

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri 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);
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141776