If the arch is P8, we will select XFLOAD to load the floating point, and then, expand it to vsx and non-vsx X-form instruction post RA. This patch is trying to convert the X-form to D-form if it meets the requirement that one operand of the x-form inst is the special Zero register, and another operand fed by add inst. i.e.
y = add imm, reg
LFDX. 0, y
-->
LFD imm(reg)
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
This needs to be beefed up to account for the two unsafe situations I outlined in the comment.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp | ||
---|---|---|
2383 ↗ | (On Diff #154364) | I just realized that this is not safe. There needs to be a check that the register being forwarded to the user isn't clobbered between DefMI and MI. Example: addis r5, r2, .LCPI0_0@toc@ha addi r4, r5, .LCPI0_0@toc@l lfdx f3, 0, r4 is safe to transform to addis r5, r2, .LCPI0_0@toc@ha lfd f3, .LCPI0_0@toc@l(r5) However, addis r6, r2, .LCPI0_1@toc@ha addis r5, r2, .LCPI0_0@toc@ha addi r4, r5, .LCPI0_0@toc@l addi r5, r6, .LCPI0_1@toc@l lfdx f3, 0, r4 is not safe to transform to addis r6, r2, .LCPI0_1@toc@ha addis r5, r2, .LCPI0_0@toc@ha addi r5, r6, .LCPI0_1@toc@l lfd f3, .LCPI0_0@toc@l(r5) because r5 that's being forwarded is already clobbered. addis r4, r2, .LCPI0_0@toc@ha addi r4, r4, .LCPI0_0@toc@l lfdx f3, 0, r4 into addis r4, r2, .LCPI0_0@toc@ha addi r4, r4, .LCPI0_0@toc@l lfd f3, .LCPI0_0@toc@l(r4) (i.e. we can only transform this case if we're sure that addi r4, r4, .LCPI0_0@toc@l will be deleted). |
Yes, you are right.
y = ADDI x, imm.
w = LFDX 0, y
We have the code to do the check for "y", but miss to do the check for "x". And this also caused some failure inside llvm test suite. (ie. Benchmarks/Stanford/FloatMM)
I will try to clean up all the failure first and then, continue the code review. Sorry for this, I should have addressed this issue early before.
Clean up all the failures from llvm test-suite. We miss to check the RegMO operand for the ADDI.
I think that this is semantically correct now. There are just a few other stylistic comments.
Also, since this is a rather high-risk patch, can you describe on the patch the functional testing you have done with the patch (list of applications, bootstrap, etc.)?
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp | ||
---|---|---|
3014 ↗ | (On Diff #156437) | s/describe/described |
3096 ↗ | (On Diff #156437) | s/reversely/in reverse |
3103 ↗ | (On Diff #156437) | I think the reader has to do a fair bit of thinking to convince themselves that this loop does the right thing. I certainly had to.
The idea would be that we simply cannot transform if we found a register def prior to DefMI in the reverse walk. Also, I don't really understand why we care about any other uses of the register being forwarded? If we have shown that the only visible def of the register is DefMI, we don't care about any uses - we just need to make sure that if DefMI->readsRegister(Reg,...), we can only transform if KilledDefMI. for (; It != E; ++It) { if (It->modifiesRegister(...) && (&*It) != DefMI) return false; // Made it to DefMI without encountering a clobber. if ((&*It) == DefMI) break; } // If DefMI also uses the register to be forwarded, we can only forward it if // DefMI is being erased. if (DefMI->readsRegister(Reg, ...)) return KillDefMI; return true; Or did I miss something and that code doesn't sufficiently specify the constraints? |
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp | ||
---|---|---|
3103 ↗ | (On Diff #156437) | You are right. If the DefMI could be erased, there won't any use between the DefMI and MI. We don't need to check the use here. |
I have done the following tests with this patch:
- LLVM check-all uniitests
- LLVM test-suites
- bootstrap build the clang with the clang including my fix, and then, run the llvm check-all and test-suites, and it passed.
The patch passed all these tests.
Thank you for the update to the patch as well as the update regarding testing that was done.
LGTM.