Page MenuHomePhabricator

Do materialize for floating point
ClosedPublic

Authored by rkotler on May 7 2014, 2:10 PM.

Details

Summary

Implement materialization of floating point constants in Mips fast-isel.

Diff Detail

Event Timeline

rkotler updated this revision to Diff 9189.May 7 2014, 2:10 PM
rkotler retitled this revision from to Do materialize for floating point.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
dsanders edited edge metadata.May 8 2014, 3:43 AM

Can you edit the summary to describe what the patch is for? It appears to be a list of commits at the moment.

lib/Target/Mips/MipsFastISel.cpp
232–249

I'd put the cast in the caller. The caller would then use something like:

if (ConstantFP *Tmp = dyn_cast<ConstantFP>(C))
  return MaterializeFP(Tmp, ...);
else if (ConstantInt *Tmp = dyn_cast<ConstantInt>(C))
  return MaterializeInt(Tmp, ...);
...
267–304

This was already in the previous patch

test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll
32–33

It isn't necessary for this patch, but just to let you know. I believe we need to make emit mthc1 to copy the upper 32-bits for the FRmodeless work

test/CodeGen/Mips/Fast-ISel/simplestorei.ll
1–63 ↗(On Diff #9189)

This was in the previous patch

rkotler updated this revision to Diff 9586.May 19 2014, 3:04 PM
rkotler edited edge metadata.
  • Merge branch '1758_6' into 1758_7
  • in progress fp merge
rkotler updated this revision to Diff 9590.May 19 2014, 3:24 PM
  • simplify a test
  • simplify call

Need to run clang-format before putback.

test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll
32–33

I'm going to change this anyway to do loads from the constant pool rather than synthesizing the literal. For sure there will be some changes to handle modeless and also when we do mips64.

test/CodeGen/Mips/Fast-ISel/simplestorei.ll
1–63 ↗(On Diff #9189)

I was not sure how to just make the delta. I surely won't commit the file again.

dsanders accepted this revision.Jun 2 2014, 2:15 AM
dsanders edited edge metadata.

LGTM with the change about putting the cast() in the caller as a dyn_cast() instead of using isa() and cast()

test/CodeGen/Mips/Fast-ISel/simplestorefp1.ll
32–33

I assume that some constants will still synthesize the literal. At minimum, 0.0 shouldn't use the constant pool.

test/CodeGen/Mips/Fast-ISel/simplestorei.ll
1–63 ↗(On Diff #9189)

'arc diff $git_revision' will make the delta from the specified revision instead of the default origin/master.

This revision is now accepted and ready to land.Jun 2 2014, 2:15 AM

I did not understand the comment about using dyn_cast instead of isa.

I will commit as is and make the update after. You can explain what you meant to me.

rkotler closed this revision.Jun 7 2014, 8:38 PM

I did not understand the comment about using dyn_cast instead of isa.

As discussed on the r210414 post-commit review thread, it seems you already made this change in r207790. The original diff for this revision passed a Constant* into MaterializeFP and used isa() in the caller, and cast() in the callee. The request was to move the cast() to the caller, combine it with the isa() to make a dyn_cast(), and change MaterializeFP to take a ConstantFP* instead of Constant*.

The patch here is rather noisy but rL210414 as committed looks good to me (apart from the commit message which should be corrected before you re-commit).

rkotler updated this object.Jun 8 2014, 12:10 PM
rkotler edited the test plan for this revision. (Show Details)
rkotler edited edge metadata.

Fixed summary and test case description.