This is an archive of the discontinued LLVM Phabricator instance.

[Sparc][LEON] Erratum fix to replace broken SDIV instruction with SDIVcc.
ClosedPublic

Authored by lero_chris on Sep 16 2016, 5:07 AM.

Details

Summary

This pass, fixing an erratum in some LEON 2 processors ensures that the SDIV instruction is not issued, but replaced by SDIVcc instead, which does not exhibit the error. Unit test included.

Diff Detail

Repository
rL LLVM

Event Timeline

lero_chris updated this revision to Diff 71618.Sep 16 2016, 5:07 AM
lero_chris retitled this revision from to [Sparc][LEON] Erratum fix to replace broken SDIV instruction with SDIVcc..
lero_chris updated this object.
lero_chris added reviewers: jyknight, jacob_hansen.
lero_chris set the repository for this revision to rL LLVM.
dcederman added inline comments.Sep 28 2016, 8:36 AM
lib/Target/Sparc/LeonPasses.cpp
300–305 ↗(On Diff #71618)

SDIVcc modifies the integer condition codes, so if any later instruction depends on the current value of those you might get the wrong result. You probably need to do the switch at an earlier stage to prevent this from happening.

lero_chris updated this revision to Diff 73655.Oct 5 2016, 9:27 AM

Addresses Daniel Cederman's concerns about the integer condition codes potentially being corrupted by marking that the ICC (condition codes) are changed by the SDIV instruction when the erratum fix is switched on. This will prevent the compiler from inserting this instruction in between the condition codes being set and later, being used.

This could be fixed in the front-end of the compiler, but it was difficult to find a working solution in the back-end except for this change.

HasLeonSdivReplace doesn't actually work the way you want it to: division gets selected manually in SparcDAGToDAGISel::Select (https://github.com/llvm-mirror/llvm/blob/master/lib/Target/Sparc/SparcISelDAGToDAG.cpp#L362), not through a pattern, so the predicate has no effect.

Actually, you should be able to substitute in the correct opcode at that point, instead of fixing up the MachineInstr later.

lero_chris updated this revision to Diff 73738.Oct 6 2016, 1:41 AM
lero_chris marked an inline comment as done.

Erratum fixed differently, in line with suggestion by Eli Friedman.

lero_chris updated this revision to Diff 73740.Oct 6 2016, 1:43 AM

Indentation in new code caused by Tab usage fixed and replaced with spaces.

dcederman edited edge metadata.Oct 6 2016, 1:56 AM

Earlier you had a unit test for this patch, but it seems to have gone missing in the later updates. Could you add it again?

lero_chris updated this revision to Diff 73744.Oct 6 2016, 2:15 AM
lero_chris edited edge metadata.

(Re-) added unit test. Added test to ensure this doesn't change behaviour for unaffected processors.

dcederman accepted this revision.Oct 7 2016, 2:42 AM
dcederman edited edge metadata.

Looks good to me now.

This revision is now accepted and ready to land.Oct 7 2016, 2:42 AM
lero_chris closed this revision.Oct 7 2016, 2:43 AM