Page MenuHomePhabricator

[MIPS] Fix useDeprecatedPositionallyEncodedOperands errors.
ClosedPublic

Authored by jyknight on Sep 19 2022, 1:20 PM.

Details

Summary

This is a follow-on to https://reviews.llvm.org/D134073.

The number of MIPS16 changes here is a bit surprising -- and even more
so because my changes have not preserved existing behavior. Many of
the fields with mismatched names were NOT previously choosing the
correct argument positionally, but instead doing something completely
wrong (e.g. it would encode a register where an immediate was
expected).

So, I would guess that the machine-code generation for MIPS16 has
never actually functioned. It's also fully untested, thus, these
changes, despite changing behavior, breaks (and fixes) zero tests.

Outside MIPS16, I believe the only functional change is to the 'ginvi'
instruction: it was previously encoding garbage into a field which was
specified to be '00'. Fortunately, it was covered by tests -- and the
tests were testing the incorrect behavior. So, fixed.

Diff Detail

Event Timeline

jyknight created this revision.Sep 19 2022, 1:20 PM
jyknight requested review of this revision.Sep 19 2022, 1:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 1:20 PM

@wzssyqa Do you mind doing some tests with this patch?

BC204 added a subscriber: BC204.Sep 21 2022, 4:06 AM

@wzssyqa Do you mind doing some tests with this patch?

We patched this patch to commit 539fa1df4634dad7d99e4485e4fc3a82ff1a5481 and tried to build it by using cmake -S llvm -B build -G Unix Makefiles` -DLLVM_ENABLE_PROJECTS=clang -DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi -DCMAKE_BUILD_TYPE=Release`. But there was an error llvm/lib/Target/Mips/Mips16InstrInfo.td:540:1: error: Illegal operand for the 'AddiuRxRyOffMemX16' instruction! def AddiuRxRyOffMemX16: when we run make V=1 VERBOSE=1 -j$(nproc).

We patched this patch to commit 539fa1df4634dad7d99e4485e4fc3a82ff1a5481 and tried to build it by using cmake -S llvm -B build -G Unix Makefiles` -DLLVM_ENABLE_PROJECTS=clang -DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi -DCMAKE_BUILD_TYPE=Release`. But there was an error llvm/lib/Target/Mips/Mips16InstrInfo.td:540:1: error: Illegal operand for the 'AddiuRxRyOffMemX16' instruction! def AddiuRxRyOffMemX16: when we run make V=1 VERBOSE=1 -j$(nproc).

Both https://reviews.llvm.org/D131003 and https://reviews.llvm.org/D134073 are prerequisites for this (in particular, missing the first one is the cause of the error.)

Both https://reviews.llvm.org/D131003 and https://reviews.llvm.org/D134073 are prerequisites for this (in particular, missing the first one is the cause of the error.)

Those have been pushed, so this patch applies successfully by itself now.

BC204 added a comment.EditedSep 27 2022, 4:12 AM

{F24716735}As of commit 539fa1df4634dad7d99e4485e4fc3a82ff1a5481
with patch https://reviews.llvm.org/D131003, https://reviews.llvm.org/D134073 and https://reviews.llvm.org/D134220.

Compiling this source code

#define R1 2 // number of rows in Matrix-1{F24716679}
#define C1 2 // number of columns in Matrix-1
#define R2 2 // number of rows in Matrix-2
#define C2 2 // number of columns in Matrix-2

int rslt[R1][C2];
void mulMat(int mat1[][C1], int mat2[][C2])
{   
    //printf("Multiplication of given two matrices is:\n");
 
    for (int i = 0; i < R1; i++) {
        for (int j = 0; j < C2; j++) {                                                                                                                                                                 
            rslt[i][j] = 0;
 
            for (int k = 0; k < R2; k++) {
                rslt[i][j] += mat1[i][k] * mat2[k][j];
            }   
 
            //printf("%d\t", rslt[i][j]);
        }   
 
        //printf("\n");
    }   
}

Thank you for the test code -- but it also fails without my change. A simpler test is just echo "int f(int a, int b) { return a * b; }" | clang -c -target mips -mips16 -xc - -o /dev/null

The cause of this failure is that the multiply instructions are -- for some reason -- marked as pseudo-instructions (see e.g. MultRxRyRz16 in Mips16InstrInfo.td). That's certainly completely wrong, and, indeed, bolsters my point that MIPS16 object file emission can't possibly have worked before.

I don't intend this change to actually fix MIPS16 object file emission -- it just makes it marginally less broken in the ways that intersect with the cross-target cleanup I'm trying to do.

So I think this change can be approved as-is.

MaskRay accepted this revision.Oct 21 2022, 1:32 PM
This revision is now accepted and ready to land.Oct 21 2022, 1:32 PM

That's certainly completely wrong, and, indeed, bolsters my point that MIPS16 object file emission can't possibly have worked before.

Sorry for the delay in responding.

Yes, MIPS16 requires the usage of the GNU assembler to work properly. When I was testing the MIPS releases for LLVM, MIPS16 required some extra handing compared to other MIPS variants because of lack of direct object emission.

This revision was landed with ongoing or failed builds.Oct 26 2022, 11:06 AM
This revision was automatically updated to reflect the committed changes.