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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson 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.
+
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:1927
     return false;
-  MinSize = 8;
+  MinSize = 8; // TODO: should this depend on -Oz?
   // On ARM11 onwards (excluding M class) 8-byte aligned LDM is typically 1
----------------
efriedma wrote:
> You want to make this more aggressive by default?  Maybe... but we probably want different heuristics for small copies.  (For example, aligning a 3-byte copy to 8 bytes makes no sense; we can't take advantage of alignment greater than 2 bytes.)
I've reverted this part of the diff and added test to show we don't adjust 3/7 byte objects


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134282