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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:967
+  if (Arg->getPointerAlignment(DL) > PrefAlign)
+    return false; // already aligned, not need to update it.
+
----------------
arichardson wrote:
> efriedma wrote:
> > 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.
> I was not aware of that function. Thanks for pointing it out that does indeed sounds like the best default.
Did you mean to address this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134282