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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 20 14:05:36 PDT 2022


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


================
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
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134282



More information about the llvm-commits mailing list