[FFmpeg-devel] [PATCH] aarch64: Add a reindentation script, check it in forgejo workflows (PR #20485)

[FFmpeg-devel] [PATCH] aarch64: Add a reindentation script, check it in forgejo workflows (PR #20485)

[Martin Storsjö via ffmpeg-devel]

PR #20485 opened by Martin Storsjö (mstorsjo)
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20485
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20485.patch

This is a script I've used for tidying up my own assembly for many years, and that I've used for touching up and making the aarch64 assembly more consistent, both in ffmpeg and other projects. Instead of keeping it privately, it's of more use to have it upstream - and it reduces the amount of things to check while reviewing aarch64 assembly, if it is checked by CI.


From cb649cfca1b17a4327303ebc670c5e88b9d301c9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin@martin.st>
Date: Thu, 15 Feb 2024 14:56:50 +0200
Subject: [PATCH 1/2] tools: Add scripts for indenting and checking aarch64
 assembly

The same also applies for arm assembly, but there are more known
deviations within that.

Add a script which checks all files, except for a few known files
that deviate, for various reasons.
---
 tools/check_arm_indent.sh    |  54 +++++++++
 tools/indent_arm_assembly.pl | 205 +++++++++++++++++++++++++++++++++++
 2 files changed, 259 insertions(+)
 create mode 100755 tools/check_arm_indent.sh
 create mode 100755 tools/indent_arm_assembly.pl

diff --git a/tools/check_arm_indent.sh b/tools/check_arm_indent.sh
new file mode 100755
index 0000000000..b8bc7b1eb7
--- /dev/null
+++ b/tools/check_arm_indent.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+#
+# Copyright (c) 2025 Martin Storsjo
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are met:
+#
+# 1. Redistributions of source code must retain the above copyright notice, this
+#    list of conditions and the following disclaimer.
+#
+# 2. Redistributions in binary form must reproduce the above copyright notice,
+#    this list of conditions and the following disclaimer in the documentation
+#    and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+
+cd $(dirname $0)/..
+
+if [ "$1" = "--apply" ]; then
+    apply=1
+fi
+
+ret=0
+
+for i in */aarch64/*.S */aarch64/*/*.S; do
+    case $i in
+        libavcodec/aarch64/h264idct_neon.S|libavcodec/aarch64/h26x/epel_neon.S|libavcodec/aarch64/h26x/qpel_neon.S|libavcodec/aarch64/vc1dsp_neon.S)
+        # Skip files with known (and tolerated) deviations from the tool.
+        continue
+    esac
+    cat $i | ./tools/indent_arm_assembly.pl > tmp.S
+    if [ -n "$apply" ]; then
+        mv tmp.S $i
+        continue
+    fi
+    if ! PAGER=cat git diff --no-index $i tmp.S; then
+        ret=1
+    fi
+done
+
+rm -f tmp.S
+
+exit $ret
diff --git a/tools/indent_arm_assembly.pl b/tools/indent_arm_assembly.pl
new file mode 100755
index 0000000000..7d5d1ecef2
--- /dev/null
+++ b/tools/indent_arm_assembly.pl
@@ -0,0 +1,205 @@
+#!/usr/bin/env perl
+#
+# Copyright (c) 2025 Martin Storsjo
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are met:
+#
+# 1. Redistributions of source code must retain the above copyright notice, this
+#    list of conditions and the following disclaimer.
+#
+# 2. Redistributions in binary form must reproduce the above copyright notice,
+#    this list of conditions and the following disclaimer in the documentation
+#    and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+
+# A script for reformatting ARM/AArch64 assembly according to the following
+# style:
+# - Instructions start after 8 columns, operands start after 24 columns
+# - Vector register layouts and modifiers like "uxtw" are written in lowercase
+# - Optionally align operand columns vertically according to their
+#   maximum width (accommodating for e.g. x0 vs x10, or v0.8b vs v16.16b).
+#
+# The input code is passed to stdin, and the reformatted code is written
+# on stdout.
+
+use strict;
+
+my $indent_operands = 0;
+my $instr_indent = 8;
+my $operand_indent = 24;
+my $match_indent = 0;
+
+while (@ARGV) {
+    my $opt = shift;
+
+    if ($opt eq "-operands") {
+        $indent_operands = 1;
+    } elsif ($opt eq "-indent") {
+        $instr_indent = shift;
+    } elsif ($opt eq "-operand-indent") {
+        $operand_indent = shift;
+    } elsif ($opt eq "-match-indent") {
+        $match_indent = 1;
+    } else {
+        die "Unrecognized parameter $opt\n";
+    }
+}
+
+if ($operand_indent < $instr_indent) {
+    die "Can't indent operands to $operand_indent while indenting " .
+        "instructions to $instr_indent\n";
+}
+
+# Return a string consisting of n spaces
+sub spaces {
+    my $n = $_[0];
+    return " " x $n;
+}
+
+sub indentcolumns {
+    my $input = $_[0];
+    my $chars = $_[1];
+    my @operands = split(/,/, $input);
+    my $num = @operands;
+    my $ret = "";
+    for (my $i = 0; $i < $num; $i++) {
+        my $cur = $operands[$i];
+        # Trim out leading/trailing whitespace
+        $cur =~ s/^\s+|\s+$//g;
+        $ret .= $cur;
+        if ($i + 1 < $num) {
+            # If we have a following operand, add a comma and whitespace to
+            # align the next operand.
+            my $next = $operands[$i+1];
+            my $len = length($cur);
+            if ($len > $chars) {
+                # If this operand was too wide for the intended column width,
+                # don't try to realign the line at all, just return the input
+                # untouched.
+                return $input;
+            }
+            my $pad = $chars - $len;
+            if ($next =~ /[su]xt[bhw]|[la]s[lr]/) {
+                # If the next item isn't a regular operand, but a modifier,
+                # don't try to align that. E.g. "add x0,  x0,  w1, uxtw #1".
+                $pad = 0;
+            }
+            $ret .= "," . spaces(1 + $pad);
+        }
+    }
+    return $ret;
+}
+
+# Realign the operands part of an instruction line, making each operand
+# take up the maximum width for that kind of operand.
+sub columns {
+    my $rest = $_[0];
+    if ($rest !~ /,/) {
+        # No commas, no operands to split and align
+        return $rest;
+    }
+    if ($rest =~ /{|[^\w]\[/) {
+        # Check for instructions that use register ranges, like {v0.8b,v1.8b}
+        # or mem address operands, like "ldr x0, [sp]" - we skip trying to
+        # realign these.
+        return $rest;
+    }
+    if ($rest =~ /v[0-9]+\.[0-9]+[bhsd]/) {
+        # If we have references to aarch64 style vector registers, like
+        # v0.8b, then align all operands to the maximum width of such
+        # operands - v16.16b.
+        #
+        # TODO: Ideally, we'd handle mixed operand types individually.
+        return indentcolumns($rest, 7);
+    }
+    # Indent operands according to the maximum width of regular registers,
+    # like x10.
+    return indentcolumns($rest, 3);
+}
+
+while (<STDIN>) {
+    # Trim off trailing whitespace.
+    chomp;
+    if (/^([\.\w\d]+:)?(\s+)([\w\\][\w\\\.]*)(?:(\s+)(.*)|$)/) {
+        my $label = $1;
+        my $indent = $2;
+        my $instr = $3;
+        my $origspace = $4;
+        my $rest = $5;
+
+        my $orig_operand_indent = length($label) + length($indent) +
+                                  length($instr) + length($origspace);
+
+        if ($indent_operands) {
+            $rest = columns($rest);
+        }
+
+        my $size = $instr_indent;
+        if ($match_indent) {
+            # Try to check the current attempted indent size and normalize
+            # to it; match existing ident sizes of 4, 8, 10 and 12 columns.
+            my $cur_indent = length($label) + length($indent);
+            if ($cur_indent >= 3 && $cur_indent <= 5) {
+                $size = 4;
+            } elsif ($cur_indent >= 7 && $cur_indent <= 9) {
+                $size = 8;
+            } elsif ($cur_indent == 10 || $cur_indent == 12) {
+                $size = $cur_indent;
+            }
+        }
+        if (length($label) >= $size) {
+            # Not enough space for the label; just add a space between the label
+            # and the instruction.
+            $indent = " ";
+        } else {
+            $indent = spaces($size - length($label));
+        }
+
+        my $instr_end = length($label) + length($indent) + length($instr);
+        $size = $operand_indent - $instr_end;
+        if ($match_indent) {
+            # Check how the operands currently seem to be indented.
+            my $cur_indent = $orig_operand_indent;
+            if ($cur_indent >= 11 && $cur_indent <= 13) {
+                $size = 12;
+            } elsif ($cur_indent >= 14 && $cur_indent <= 17) {
+                $size = 16;
+            } elsif ($cur_indent >= 18 && $cur_indent <= 22) {
+                $size = 20;
+            } elsif ($cur_indent >= 23 && $cur_indent <= 27) {
+                $size = 24;
+            }
+            $size -= $instr_end;
+        }
+        my $operand_space = " ";
+        if ($size > 0) {
+            $operand_space = spaces($size);
+        }
+
+        # Lowercase the aarch64 vector layout description, .8B -> .8b
+        $rest =~ s/(\.[84216]*[BHSD])/lc($1)/ge;
+        # Lowercase modifiers like "uxtw" or "lsl"
+        $rest =~ s/([SU]XT[BWH]|[LA]S[LR])/lc($1)/ge;
+
+        # Reassemble the line
+        if ($rest eq "") {
+            $_ = $label . $indent . $instr;
+        } else {
+            $_ = $label . $indent . $instr . $operand_space . $rest;
+        }
+    }
+    print $_ . "\n";
+}
-- 
2.49.1


From 556b6ba99cbdcc77f132bcfcd7ea93a5226f4705 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin@martin.st>
Date: Wed, 10 Sep 2025 12:26:53 +0300
Subject: [PATCH 2/2] forgejo: Check the aarch64 assembly indentation as part
 of the lint job

Alternatively, this could be a separate job, potentially keyed
to only run on PRs that touch files matching */aarch64/*. But
as this runs very quickly, it's probably less clutter to just
bundle it here.
---
 .forgejo/workflows/lint.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.forgejo/workflows/lint.yml b/.forgejo/workflows/lint.yml
index 42e925ad8b..afbdba89a2 100644
--- a/.forgejo/workflows/lint.yml
+++ b/.forgejo/workflows/lint.yml
@@ -24,3 +24,5 @@ jobs:
           key: pre-commit-${{ steps.install.outputs.envhash }}
       - name: Run pre-commit CI
         run: ~/pre-commit/bin/pre-commit run -c .forgejo/pre-commit/config.yaml --show-diff-on-failure --color=always --all-files
+      - name: Check aarch64 assembly indentation
+        run: ./tools/check_arm_indent.sh
-- 
2.49.1

_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org