[PATCH] D134282: [CGP] Add generic TargetLowering::shouldAlignPointerArgs() implementation

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:1925
+  virtual bool shouldUpdatePointerArgAlignment(const CallInst *CI,
+                                               const Value *Arg,
+                                               unsigned &MinSize,
----------------
efriedma wrote:
> It looks like the argument "Arg" is now unused?
I was using it for allowsMisalignedMemoryAccesses() alignment, but dropped it in the previous diff.
I think having the current argument that is being processed could be useful for overrides (since they could make decisions based on the current alignment).

I've added the use back now, but am also happy to drop it.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:1925
+  virtual bool shouldUpdatePointerArgAlignment(const CallInst *CI,
+                                               const Value *Arg,
+                                               unsigned &MinSize,
----------------
efriedma wrote:
> arichardson wrote:
> > efriedma wrote:
> > > It looks like the argument "Arg" is now unused?
> > I was using it for allowsMisalignedMemoryAccesses() alignment, but dropped it in the previous diff.
> > I think having the current argument that is being processed could be useful for overrides (since they could make decisions based on the current alignment).
> > 
> > I've added the use back now, but am also happy to drop it.
> Still here?  Did you mean to upload a different change?
Arg is used in the call to allowsMisalignedMemoryAccesses(getPointerMemTy() to determine if it's already a fast operation.
I can drop it if you prefer, I just thought it is potentially useful to avoid additional aligning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134282