This is an archive of the discontinued LLVM Phabricator instance.

Implement FastMaterializeAlloca in Mips fast-isel
ClosedPublic

Authored by vkalintiris on Dec 19 2014, 2:05 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

rkotler updated this revision to Diff 17518.Dec 19 2014, 2:05 PM
rkotler retitled this revision from to Implement FastMaterializeAlloca in Mips fast-isel.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler added subscribers: Unknown Object (MLST), rfuhler.
vkalintiris commandeered this revision.Apr 16 2015, 3:44 AM
vkalintiris added a reviewer: rkotler.

Add explicit type parameter to load/GEP instructions.

dsanders accepted this revision.Apr 17 2015, 9:20 AM
dsanders edited edge metadata.

LGTM with some changes

lib/Target/Mips/MipsFastISel.cpp
260–263 ↗(On Diff #23843)

This is redundant. DenseMapBase<...>::find() returns DenseMapBase<...>::end() when the key isn't found so there's no need to separately count(AI) and find(AI)

test/CodeGen/Mips/Fast-ISel/fastalloca.ll
4–7 ↗(On Diff #23843)

Please drop these four lines from the test. It's better to specify the triple on the command line and using the default datalayout will be better if it ever needs to change.

(sorry for the duplicate comment, Phabricator won't let me delete either of them for some reason)

16–29 ↗(On Diff #23843)

I think we could simplify this test quite a lot. We only want to test alloca instructions so I think we could do something like:

define i32 @foo(i32* %d) {
  %0 = alloca i32, align 4
  store i32 %0, i32* %d, align 4
}

and just check that appropriate addiu's and sw's are created. Obviously, we'd want to test a few layouts.

That said, we've discussed (off-list) your plan to migrate these tests to a style similar to the test/CodeGen/Mips/llvm-ir tests (which tries to test each LLVM-IR instruction with as few dependencies as possible) once the backlog is cleared and I'm happy for this kind of change to be made at that point.

31 ↗(On Diff #23843)

Nit: indentation

39–57 ↗(On Diff #23843)

Nit: Not needed.

3–6 ↗(On Diff #17518)

Please drop these four lines from the test. It's better to specify the triple on the command line and using the default datalayout will be better if it ever needs to change.

This revision is now accepted and ready to land.Apr 17 2015, 9:20 AM
This revision was automatically updated to reflect the committed changes.