[PATCH] D148622: [LoongArch] Align functions and loops better according to uarch

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n marked 2 inline comments as done.
xen0n added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:200
+  // Set preferred alignments.
+  setPrefFunctionAlignment(STI.getPrefFunctionAlignment());
+  setPrefLoopAlignment(STI.getPrefLoopAlignment());
----------------
SixWeining wrote:
> Almost all parts of this constructor use the member variable `Subtarget` but not the parameter `STI` except line 187 which I will change it later.
Okay I'll change to `Subtarget` for consistency with the local style shortly. (Although IMO local variables/arguments should be preferred over instance states which is more global, it's best done separately.)


================
Comment at: llvm/lib/Target/LoongArch/LoongArchSubtarget.cpp:38
   ParseSubtargetFeatures(CPU, TuneCPU, FS);
+  initializeProperties();
   if (Is64Bit) {
----------------
SixWeining wrote:
> Should we pass `CPU` or `TuneCPU` to this function and use it to set different numbers?
For now I've added the `TuneCPU` argument for smaller future diffs when we get to add more uarch data later.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchSubtarget.h:55
+  Align PrefLoopAlignment;
+  unsigned MaxBytesForLoopAlignment = 0;
 
----------------
SixWeining wrote:
> Is this initial value useful? `PrefFunctionAlignment` and `PrefLoopAlignment` do not have initial values?
This is similar to the way AArch64 does it (that I obviously referred to). Apparently the default ctor of `Align` means "byte alignment" and zero MaxBytesForAlignment means disabling that feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148622