This is an archive of the discontinued LLVM Phabricator instance.

Finish materialize for ints
ClosedPublic

Authored by rkotler on May 2 2014, 1:56 PM.

Details

Summary

We add code to materialize all integer literals.

Diff Detail

Event Timeline

rkotler updated this revision to Diff 9043.May 2 2014, 1:56 PM
rkotler retitled this revision from to Finish materialize for ints.
rkotler updated this object.
rkotler added a reviewer: dsanders.
dsanders added inline comments.May 8 2014, 2:27 AM
lib/Target/Mips/MipsFastISel.cpp
246–274

You could share more code between the paths using something like:

unsigned Hi = (Imm >> 16) & 0xffff;
unsigned Lo = Imm & 0xffff;

if (Hi && !isImm<16>(Imm)) {
  BuildMI(..., TII.get(Mips::LUi), ...)...
  Imm &= 0xffff;
}
if (Lo) {
  unsigned Opcode = isImm<16>(Imm) ? Mips::ADDiu : Mips::ORi)
  BuildMI(..., TII.get(Opcode), ...)...
}
test/CodeGen/Mips/Fast-ISel/simplestorei.ll
11–13

Inconsistent indentation. It happens in the other functions too.

rkotler added inline comments.May 8 2014, 10:08 AM
lib/Target/Mips/MipsFastISel.cpp
246–274

I see your point now. We can change it as you suggest. I just need to add some comments.

rkotler added inline comments.May 8 2014, 10:16 AM
lib/Target/Mips/MipsFastISel.cpp
246–274

Upon further reflection, I think that what I have now is very clear.

I don't think we are adding anything from reducing a few lines of code.
It will, IMO, just make it more difficult to understand.

Right now there are four case:
16 bit (signed and unsigned), 32 bit with lower have 0, 32 bit unrestricted

I would like to go back though and make some better BuildMI functions because we are repeating too many of the same parameters.

rkotler updated this object.May 8 2014, 11:06 AM
rkotler edited the test plan for this revision. (Show Details)
rkotler updated this revision to Diff 9229.May 8 2014, 11:26 AM
  • untabify test case
dsanders accepted this revision.May 14 2014, 7:12 AM
dsanders edited edge metadata.

LGTM with the indentation nits.

lib/Target/Mips/MipsFastISel.cpp
246–274

I disagree but it's very subjective and I don't have a strong argument in favour of either style. I suspect that you'll find the number of cases grows significantly when support for 64-bit immediates is added (particularly with MIPS64r6). We can refactor if/when that happens.

This revision is now accepted and ready to land.May 14 2014, 7:12 AM

The indentation nits are already fixed. LGTM.

rkotler closed this revision.May 15 2014, 3:01 PM