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.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| llvm/lib/Target/PowerPC/PPCInstrInfo.cpp | ||
|---|---|---|
| 2097 | 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();
}
#endifand use that in this (and similar) assert(s). | |
Comment Actions
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;
}
Comment Actions
There are two changes:
- add a query to check if it is imm operand as Nemanjai suggested. Didn't find other places that could use this query.
- add a new test case to address the global case.
Comment Actions
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 | 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:
| |
| 4 | Nit: indentation. | |
| 12 | 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. | |
Comment Actions
Other than the minor nit, LGTM.
| llvm/test/CodeGen/PowerPC/toc-float.ll | ||
|---|---|---|
| 66 | 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. | |
| llvm/test/CodeGen/PowerPC/toc-float.ll | ||
|---|---|---|
| 66 | 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. | |
| llvm/test/CodeGen/PowerPC/toc-float.ll | ||
|---|---|---|
| 66 | 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. | |
| llvm/test/CodeGen/PowerPC/toc-float.ll | ||
|---|---|---|
| 5 | 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. | |
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(); } #endifand use that in this (and similar) assert(s).