Page MenuHomePhabricator

[Mips] Use UADDO/ADDCARRY instead of ADDC/ADDE
Needs ReviewPublic

Authored by deadalnix on Jun 4 2018, 7:25 AM.

Details

Summary

This changes the DAG to DAG to lower from UADDO/ADDCARRY. It involves some trickery to make sure we do not materialize the carry when it comes from ADDSC.

The spec doesn't mention if ADDWC clear with 20, however, code clearing it was removed as The carry now always comes from a ADDSC and previous version of the code did not seems to worry about bit 20 being set in that case, so my assumption is that this is the proper thing to do.

Diff Detail

Event Timeline

deadalnix created this revision.Jun 4 2018, 7:25 AM

I take it that this patch is dependent on D47703 ?

The legalization of unsigned i128 addition (rL334094 for the test) looks strange to me in that the determination of the overflow from the first addition is done with setccs. They appear to be originating from the expansion of setcc of an illegal type.

I'm not entirely convinced on performing addsc/addwc sequences over addsc/(multiple addwc) sequences. The overflow bit for the addwc has to be cleared after reading it so that the next addwc in either type of expansion delivers the correct result. At which point you might as well shift that bit into the carry bit in the dsp control register and perform addwc again.

Some additional comments inline.

lib/Target/Mips/MipsSEISelDAGToDAG.cpp
255

I realised that I got this wrong when I wrote it. This code should be using 0x1f to read the entire register.

295

This has to be materialized into a register as addsc doesn't have an immediate form, use ADDiu, -1 to materialize it.

310

You also need to clear bit 20 in the dsp control register after performing a addwc. On page 40 of the MIPS DSP AFP (MD00374-2B-MIPS32DSP-AFP-03.01.pdf) , it notes that the over/underflow flags are sticky.

deadalnix added inline comments.Jun 6 2018, 3:08 PM
lib/Target/Mips/MipsSEISelDAGToDAG.cpp
310

Question is, does addsc clean the flag register ? If yes, then we are good, if not, then existing code is broken as well.

sdardis added inline comments.Jun 12 2018, 9:13 AM
lib/Target/Mips/MipsSEISelDAGToDAG.cpp
310

By the spec from my reading, addsc only writes the carry bit (bit 13) on every execution and bit 20 is untouched.

I do not see any test cases in this patch. Is it intended?

@atanasyan The behavior is not supposed to change in any way, so test cases should not change. However, there are doubt about semantic for which there is no test case today.