Page MenuHomePhabricator

[LegalizeDAG][Mips] Add an assert to protect a uint_to_fp implementation from double rounding. Add a i32->f32 uint_to_fp implementation that avoids this code.
ClosedPublic

Authored by craig.topper on Jan 15 2020, 11:24 AM.

Details

Summary

The algorithm here only works if the sint_to_fp doesn't do any rounding. Otherwise it can round before the offset fixup is applied. Add an assert to protect this.

One test in tree was using this code for i32->f32 when f64 isn't legal. That test was only checking for not crashing until an earlier commit today. I've added a i32->f32 conversion using the same code as i64->f32 from TargetLowering. This is similar to what Mips gcc does for this config according to godbolt except they use control flow instead of selects. We should merge the new code with the code in TargetLowering, but the order of things is a little tricky right now. We call TLI first, then some code earlier in LegalizeDAG that tries to use i32->f32 by using i32->f64->f32. This new i32->f32 needs to be lower priority than i32->f64->f32. So I'm focusing on correctness first before refactoring this.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 15 2020, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 11:24 AM
craig.topper edited the summary of this revision. (Show Details)Jan 15 2020, 11:25 AM

Why do we need to copy this algorithm here? Can't you simply add this case to TargetLowering::expandUINT_TO_FP instead?

Why do we need to copy this algorithm here? Can't you simply add this case to TargetLowering::expandUINT_TO_FP instead?

Ah, nevermind -- I see you're talking about this in the summary ...

I was able to move the other version of this code from TargetLowering back to LegalizeDAG. So just need to add the new type to it.

This revision is now accepted and ready to land.Jan 16 2020, 4:29 AM
This revision was automatically updated to reflect the committed changes.