This is an archive of the discontinued LLVM Phabricator instance.

[Power9] Do the add-imm peephole for pseudo instruction DFLOADf32/DFLOADf64 and the store pair
ClosedPublic

Authored by steven.zhang on May 30 2018, 7:51 PM.

Details

Summary

If the arch is P9, we will select the DFLOADf32/DFLOADf64 pseudo instruction when we are loading a floating, and expand it post RA basing on the register pressure. However, we miss to do the add-imm peephole for these pseudo instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

steven.zhang created this revision.May 30 2018, 7:51 PM
nemanjai requested changes to this revision.May 31 2018, 1:43 AM
nemanjai added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2091 ↗(On Diff #149228)

This is not adequate. We don't only convert this for constant pool loads, it will crash with something like this (also please add this as a test case):

float FArr[10];

float getF() {
  return FArr[3] + 3.4f;
}

I think it's probably a good idea to implement something like:

#ifndef NDEBUG
static bool isAnImmediateOperand(const MachineOperand &MO) {
  return MO.isCPI() || MO.isGlobal() || MO.isImm();
}
#endif

and use that in this (and similar) assert(s).

This revision now requires changes to proceed.May 31 2018, 1:43 AM
steven.zhang added a comment.EditedMay 31 2018, 7:17 PM

Thank you for the comment. I will fix that. BTW, I cannot get the assertion from your case, but this case.(miss some options ?) It seems that, clang put the floating variable into the TOC with some condition, which is different from gcc/xlc. I will deliver some other change to fix this issue.

float attribute((visibility("hidden"))) FArr[10];

float getF() {

return FArr[3] + 3.4f;

}

steven.zhang updated this revision to Diff 149393.EditedMay 31 2018, 8:17 PM

There are two changes:

  1. add a query to check if it is imm operand as Nemanjai suggested. Didn't find other places that could use this query.
  2. add a new test case to address the global case.

I'd typically be OK with these cosmetic changes being made on the commit, but there are a large enough number of changes that I'd prefer to see the updated patch. Thanks for fixing these.

llvm/test/CodeGen/PowerPC/toc-float.ll
1 ↗(On Diff #149393)

Nit: There's only one RUN in this test case, no need for the check prefix - just use the default CHECK directives with no prefix. Also, it would be good to have all the following tests:

  • Returning a double constant that can be represented as float (you already have this)
  • Returning a double constant that cannot be represented as float (you already have this)
  • Returning a float constant
  • Accessing a global array of float (you already have this)
  • Accessing a global array of double
  • Accessing a global array of either double or float where the index of the access is large enough that a D-Form load cannot be used (perhaps above 4096 for double)
4 ↗(On Diff #149393)

Nit: indentation.

12 ↗(On Diff #149393)

Nit: please change the constant being returned to be significantly different from the one being returned from bar() so that it is obvious from a quick visual inspection that they're different. Also, you should add a comment that the constant cannot be represented exactly as float so a double is loaded from the constant pool. This can either be with a comment or by naming the function adequately.

steven.zhang marked 3 inline comments as done.Jun 8 2018, 12:49 AM

Update the change basing on Nemanjai's comment. Thank you.

nemanjai accepted this revision.Jun 8 2018, 3:14 PM

Other than the minor nit, LGTM.

llvm/test/CodeGen/PowerPC/toc-float.ll
65 ↗(On Diff #150470)

This comment is unclear to me. I'm not sure how we could use a D-Form instruction when the offset doesn't fit in the displacement field. In any case, clarify the comment or remove it.

This revision is now accepted and ready to land.Jun 8 2018, 3:14 PM
steven.zhang added inline comments.Jun 10 2018, 7:25 PM
llvm/test/CodeGen/PowerPC/toc-float.ll
65 ↗(On Diff #150470)

LFD didn't have the alignment restrict while LXSD has. When TOC_Entry is lowing, we assume that, the disp must be multiple of 4, which in fact, is not necessary for LFD. Therefore, we could do it post RA. For this case, LFD 32768[REG] is valid.

steven.zhang marked an inline comment as done.Jun 10 2018, 10:03 PM
steven.zhang marked an inline comment as done.Jun 10 2018, 10:10 PM
steven.zhang added inline comments.
llvm/test/CodeGen/PowerPC/toc-float.ll
65 ↗(On Diff #150470)

Sorry, please just ignore this comment, as I didn't realize that, the imm for LFD is signed, whose range is -32768 ~ 32767. LFD 32768[REG] is invalid. And this has nothing to do with alignment.

steven.zhang marked 2 inline comments as done.Jun 10 2018, 10:23 PM

Add the triple for the new created test case toc-float.ll

nemanjai added inline comments.Jun 12 2018, 3:48 PM
llvm/test/CodeGen/PowerPC/toc-float.ll
4 ↗(On Diff #150897)

The preference is to specify the triple on the command line in test cases and not in the IR. Also, please remove the datalayout.

Of course, feel free to do this on the commit.

Remove the data layout and triple in the IR and specify it in the command line.

This revision was automatically updated to reflect the committed changes.