This is an archive of the discontinued LLVM Phabricator instance.

Model sqrtss as a binary operation with one source operand tied to the destination (PR14221)
ClosedPublic

Authored by spatel on Nov 19 2014, 3:00 PM.

Details

Summary

This is a continuation of r167064 ( http://llvm.org/viewvc/llvm-project?view=revision&revision=167064 ).
That patch started to fix PR14221 ( http://llvm.org/bugs/show_bug.cgi?id=14221 ), but was not completed, so we have a chunk of extra code that is preventing further improvements.

If everything looks ok here, I can submit a follow-on patch to fix sqrtsd (double-precision square root).

I'm not locating a Phab address for Manman Ren, so I'll send a follow-on mail.

Diff Detail

Event Timeline

spatel updated this revision to Diff 16406.Nov 19 2014, 3:00 PM
spatel retitled this revision from to Model sqrtss as a binary operation with one source operand tied to the destination (PR14221).
spatel updated this object.
spatel edited the test plan for this revision. (Show Details)
spatel added reviewers: hliao, rafael.
spatel added a subscriber: Unknown Object (MLST).
spatel added a comment.Dec 5 2014, 8:52 AM

Ping * 2.

...we are miscompiling code without this patch as shown in http://llvm.org/bugs/show_bug.cgi?id=14221 .

Is there anything stopping the sse1_fp_unop_* and sse2_fp_unop_* defs being merged into sse12_fp_unop_* like the binops already have been? SQRTSD is the only reference to sse2_fp_unop_s and generally there is a high level of code duplication between the 2 sets of defs that the sse12_ types reduce a lot.

Is there anything stopping the sse1_fp_unop_* and sse2_fp_unop_* defs being merged into sse12_fp_unop_* like the binops already have been? SQRTSD is the only reference to sse2_fp_unop_s and generally there is a high level of code duplication between the 2 sets of defs that the sse12_ types reduce a lot.

I was planning to fix sqrtsd in a follow-on patch. I agree that there's more consolidation possible, and I don't think there's anything that would prevent handling the unop cases similarly to the binop cases.

delena edited edge metadata.Dec 19 2014, 11:46 AM

I checked this patch and it LGTM.

  • Elena
hliao edited edge metadata.Dec 19 2014, 1:18 PM

LGTM. Sorry for the really LATE reply.

This revision was automatically updated to reflect the committed changes.

Thanks, Elena and Michael! Checked in with r224624.