[PATCH] D114879: [AArch64][CodeGen] Emit alignment "Max Skip" operand for AArch64 loops

Tue Dec 14 00:27:54 PST 2021

dmgreen added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8529

+Align AArch64TargetLowering::getPrefLoopAlignment(MachineLoop *ML) const {
+  // TODO Investigate how to make this better, this is not ready to commit
----------------
dmgreen wrote:
> The alignments in AArch64 are usually set in MachineBlockPlacement::alignBlocks, which is probably the more general place for this. The alignments can then be set in the same places as setPrefLoopAlignment for each backend.
> 
> Maybe at the moment, before we've fixed on proper values for AArch64 cpus, we could use an option as a way of testing that the values are getting propagated correctly.
Do we need to override this method, or can it work more consistently like TargetLoweringBase::setPrefLoopAlignment and getPrefLoopAlignment? That way we can set the value in AArch64TargetLowering::AArch64TargetLowering like we already do for other loop alignments, and keep everything together.

The (LoopAlign, MaxBytesForLoopAlign) can be thought of as a pair. I think it makes sense to keep them as separate variables, but ideally they are set and used together.

================
Comment at: llvm/test/CodeGen/AArch64/aarch64-p2align-max-bytes-neoverse.ll:11
+; CHECK-NEXT:  .LBB0_8: // %for.body
+; CHECK-OBJ;Disassembly of section .text:
+; CHECK-OBJ:      88: 2a 00 0a 8b   add
----------------
This file needn't have the objfile checks, so long as they are tested elsewhere.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114879