This is an archive of the discontinued LLVM Phabricator instance.

Add load/store functionality
ClosedPublic

Authored by rkotler on May 20 2014, 3:55 PM.

Details

Summary

This patches allows non conversions like i1=i2; where both are global ints.
In addition, arithmetic and other things start to work since fast-isel will use
existing patterns for non fast-isel from tablegen files where applicable.

In addition i8, i16 will work in this limited context for assignment without the need
for sign extension (zero or signed). It does not matter how i8 or i16 are loaded (zero or sign extended)
since only the 8 or 16 relevant bits are used and clang will ask for sign extension before using them in
arithmetic. This is all made more complete in forthcoming patches.

for example:

int i, j=1, k=3;
 
void foo() {
  i = j + k;
}

Keep in mind that this pass is not enabled right now and is an experimental pass
It can only be enabled with a hidden option to llvm of -mips-fast-isel.

Diff Detail

Event Timeline

rkotler updated this revision to Diff 9645.May 20 2014, 3:55 PM
rkotler retitled this revision from to Add load/store functionality.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler updated this revision to Diff 9662.May 21 2014, 10:34 AM
  • fix register class

The following test case executes correctly:

~/llvmw/build/Debug+Asserts/bin/clang -target mips-linux-gnu -mips32r2 -mllvm -fast-isel -O0 -mllvm -mips-fast-isel -mllvm -fast-isel-verbose loadstore2.c -fPIC -gcc-toolchain /mips/proj/build-compiler/stable-nightly-gcc/install-mips-mti-linux-gnu -EL

/mips/arch/overflow/codesourcery/mips-linux-gnu/pro/release/2011.09-90/Linux/bin/mips-linux-gnu-qemu-el -r 2.6.38 -E LD_LIBRARY_PATH=/mips/proj/build-compiler/stable-nightly-gcc/install-mips-mti-linux-gnu/sysroot/el/usr/lib:/mips/proj/build-compiler/stable-nightly-gcc/install-mips-mti-linux-gnu/sysroot/el/usr/usr/lib:/mips/proj/build-compiler/stable-nightly-gcc/install-mips-mti-linux-gnu/mips-mti-linux-gnu/lib/el -L /mips/proj/build-compiler/stable-nightly-gcc/install-mips-mti-linux-gnu/sysroot/el/usr a.out

char c1, c2=6;
short s1, s2=123;
int i1, i2=65536;
float f1, f2=1.978;
double d1, d2=1.23456789876;

void cfoo() {

c1=c2;

}

void sfoo() {

s1=s2;

}

void ifoo() {

i1=i2;

}

void ffoo() {

f1=f2;

}

void dfoo() {

d1=d2;

}

int main() {

cfoo();
sfoo();
ifoo();
ffoo();
dfoo();
printf("%i %i %i %f %f \n", c1, s1, i1, f1, d1);

}

This simple test with plus will also work now. I need to go through all the operators more in depth.

int i, j=1, k=3;

void foo() {

i = j + k;

}

int main() {

foo();
printf("%i \n", i);

}

dsanders added inline comments.Jun 11 2014, 2:58 AM
lib/Target/Mips/MipsFastISel.cpp
170–211

Is this if-statement correct? It looks like we are rejecting sign-extended loads here (or possibly any-extended), but below we are picking the sign-extended load instructions.

It also seems that IsZExt is always true at the moment. As far as I can tell no caller passes it in and the default value is true.

215–233

Whitespace nit around the '='.

Can you also fix the ones nearby (in another patch)?

test/CodeGen/Mips/Fast-ISel/loadstore2.ll
25–28

The first operand to lb/sb should be the same register. Please use a variable to check for this too like so:

; CHECK: lb $[[R0:[0-9]+]], 0(${{[0-9]+}})
; CHECK: sb $[[R0]], 0(${{[0-9]+}})

Likewise for the other tests.

Also, one thing that didn't occur to me on the earlier patches is that '; CHECK: .ent cfoo' should be '; CHECK-LABEL: .ent cfoo'. The advantage of using CHECK-LABEL is that FileCheck isn't limited to reporting a single mismatch in the file. Instead it is limited to one mismatch per CHECK-LABEL.

rkotler updated this revision to Diff 10429.Jun 15 2014, 2:40 PM

refresing patch

Executable test cases.

/home/rkotler/llvm-push/build/Debug+Asserts/bin/clang -target mips-linux-gnu -mips32r2 -mllvm -fast-isel -O2 -mllvm -mips-fast-isel -mllvm -fast-isel-verbose loadstoreu2.c -fPIC -gcc-toolchain /mips/proj/build-compiler/stable-nightly-gcc/install-mips-mti-linux-gnu -EL -save-temps -o loadstoreu2

rkotler updated this revision to Diff 10432.Jun 15 2014, 3:01 PM
  • clang-format
dsanders edited edge metadata.Jun 16 2014, 6:13 AM

Thanks for refreshing the patch to remove the noise.

This patches allows non coversions like i1=i2; where both are ints
In addition, arithmetic and other things start to work.
for example:

int i, j=1, k=3;

void foo() {
  i = j + k;
}

For a moment after reading this, I was wondering how support for load instructions could make arithmetic work :-). I understand that this patch adds support for the global variable load and that combined with the existing support for arithmetic the whole example should now work. The summary could be a lot clearer about what the patch changes compared to what begins to work as a result of the total work so far. Also, you should mention that you added support for i8 and i16 stores in this patch.

By the way, to format code examples properly in the web interface you need to put at least two spaces at the start of each line.

LGTM with a clearer commit message (that mentions the i8/i16 store change) and the EmitLoadMem vs EmitInstStore nit fixed.

lib/Target/Mips/MipsFastISel.cpp
89

Nit: For consistency with EmitInstStore, you need to either drop the 'Mem' here or add it to EmitInstStore.

170–211

This has been fixed. The IsZExt argument has been dropped and EmitLoad now uses the unsigned loads.

test/CodeGen/Mips/Fast-ISel/loadstore2.ll
25–28

This has been fixed

rkotler updated this object.Jun 16 2014, 10:56 AM
rkotler edited edge metadata.
rkotler updated this object.
rkotler updated this object.Jun 16 2014, 11:19 AM
rkotler updated this revision to Diff 10452.Jun 16 2014, 11:26 AM
  • Merge branch 'master' into 1758_8a
  • fix naming issue
rkotler updated this object.Jun 16 2014, 2:42 PM
rkotler closed this revision.Jun 16 2014, 3:14 PM
rkotler updated this revision to Diff 10462.

Closed by commit rL211061 (authored by @rkotler).