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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment 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);
+    }
----------------
pengfei wrote:
> 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?
Does anyone have comments on the rest of the change, ignoring this particular function? :)

If i drop this function, we get compilation error.
We can't make it virtual because then every target
will need to override every single `allowsMemoryAccess()`,
which is error-prone, as comparing a single `allowsMemoryAccess()`.

I do not understand the feedback here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141776