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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:1925
+  virtual bool shouldUpdatePointerArgAlignment(const CallInst *CI,
+                                               const Value *Arg,
+                                               unsigned &MinSize,
----------------
arichardson wrote:
> 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.
I'm not sure how calling getPointerAlignment() avoids additional alignment, assuming PrefAlign is correct. shouldAlignPointerArgs is supposed to return the minimum "fast" alignment for the given call.  If the actual alignment is already greater than or equal to that, the caller does nothing, even if shouldAlignPointerArgs returns true.

(If there's some alignment between 1 and PrefAlign that the target considers "fast", I guess you could run into an issue, but that implies PrefAlign is wrong.)


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:967
+  if (Arg->getPointerAlignment(DL) > PrefAlign)
+    return false; // already aligned, not need to update it.
+
----------------
arichardson wrote:
> JojoR wrote:
> > efriedma wrote:
> > > The question is what size load/store ops would we prefer to use for the memcpy, and whether those ops require alignment.  Using the alignment of a pointer seems arbitrary; we aren't loading or storing a pointer.
> > > 
> > > I'd prefer not to call getPointerAlignment() here if we can avoid it; the caller already does the math to figure out the current alignment and the increase.  shouldUpdatePointerArgAlignment just needs to know what alignment it wants, not whether the call currently satisfies that alignment.
> > As @efriedma said, we can set default PrefAlign according to PointerSize,
> > but we should return final PrefAlign from backend du to backend's requirement,
> > different ISAs maybe have different alignment :)
> I agree that this is a backend-specific choice. I would assume loading an aligned pointer is an efficient operation on most/all targets, and for the ones where this is not true, they can override `shouldUpdatePointerArgAlignment()`. 
> 
> I believe this change should match now what you did for RISC-V: MinSize==XLEN PrefAlign==XLEN.
Maybe TargetTransformInfo::getRegisterBitWidth() is a better default than getPointerPrefAlignment()?  I guess that's the same thing for most targets, but it probably makes the intent a bit more clear...

I agree there isn't any default that's going to be correct for all targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134282