Skip to content

[PPC64] Define sign conditions for each block#7326

Closed
lbianc wants to merge 1 commit intofacebook:masterfrom
PPC64:for-facebook-def-conditions-PPC64
Closed

[PPC64] Define sign conditions for each block#7326
lbianc wants to merge 1 commit intofacebook:masterfrom
PPC64:for-facebook-def-conditions-PPC64

Conversation

@lbianc
Copy link
Contributor

@lbianc lbianc commented Sep 9, 2016

On PPC64 there is more than one Condition Register and depending if the
operation is signed or unsigned, a different CR is set.
After the VASM emission and all optimization phases, the new "Define
Conditions" phase is called to check if the block has signed, unsigned,
neither or both kind of operations, so based on that, the correct
comparing instruction is emitted.

@lbianc
Copy link
Contributor Author

lbianc commented Sep 15, 2016

@mxw this PR has on check pending since I have created it, is there any issue with that?

@ghost ghost added the CLA Signed label Sep 15, 2016
@aorenste
Copy link
Contributor

@lbianc - weird. The internal tool shows all tests passed. @fredemmott any idea why fb/FB tests is still listed as "In progress"?

@ghost ghost added the CLA Signed label Sep 15, 2016
@fredemmott
Copy link
Contributor

Bug - we only create/update an 'FB tests' entry if state !== ok

@fredemmott
Copy link
Contributor

fredemmott commented Sep 15, 2016

Fix ready, ran for just this PR. Problem will still happen on others or if you update this for a while.

@lbianc
Copy link
Contributor Author

lbianc commented Sep 16, 2016

@fredemmott thanks!

@ghost ghost added the CLA Signed label Sep 16, 2016
@mxw
Copy link
Contributor

mxw commented Sep 20, 2016

Can you check out the comment I left here: #7348 (comment)

I feel like exposing the XLS liveness analysis for SF registers would also be something that you could use here. If you agree, let's hold off on this until I refactor that analysis so that you can use it.

@lbianc
Copy link
Contributor Author

lbianc commented Sep 20, 2016

Yes, seems something that I can use here. No problem wait for that.

One question, with this XLS liveness analysis of SF registers, will I be able to know which instructions use it during the live period?

On PPC64 there is more than one Condition Register and depending if the
operation is signed or unsigned, a different CR is set.
After the VASM emission and all optimization phases, the new "Define
Conditions" phase is called to check if the block has signed, unsigned,
neither or both kind of operations, so based on that, the correct
comparing instruction is emitted.
@lbianc lbianc force-pushed the for-facebook-def-conditions-PPC64 branch from 2be0cea to 712834c Compare September 22, 2016 16:04
@lbianc
Copy link
Contributor Author

lbianc commented Sep 22, 2016

Updated to solve conflicts with master.

@hhvm-bot
Copy link
Contributor

@lbianc updated the pull request - view changes - changes since last import

@ghost ghost added the CLA Signed label Sep 22, 2016
@mxw
Copy link
Contributor

mxw commented Sep 22, 2016

@lbianc—Yes, I'll make it so that you can access the use-instructions.

@ghost ghost added the CLA Signed label Sep 22, 2016
@mxw
Copy link
Contributor

mxw commented Oct 4, 2016

Okay, I spent some time working on a separate pass for this, but ended up just deciding to use vasm-simplify.cpp. I think that's a better place for this sort of analysis than the emitter, and you can add some flags to Vinstr (there are a few empty bytes) as annotation instead of creating even more ppc64-specific Vinstrs (I think it's best to minimize the number of those where possible).

I would add an extra data structure to the Env in vasm-simplify.cpp, and do an analysis pass that identifies the set of ConditionCodes that each VregSF might be tested with. This can be a conservative analysis, so that you don't have to update the cc-use-set as simplifications are made. Let me know if you think that sounds like it will work for you.

@mxw
Copy link
Contributor

mxw commented Oct 10, 2016

Closing this PR in favor of a potential future implementation of a solution like the one described above.

@mxw mxw closed this Oct 10, 2016
@gut gut deleted the for-facebook-def-conditions-PPC64 branch July 7, 2017 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants