Page MenuHomePhabricator

[PowerPC] Exploit splat instruction xxsplti32dx in Power10

Authored by Conanap on Oct 26 2020, 10:49 AM.


Group Reviewers
Restricted Project

Exploits the instruction xxsplti32dx.

It can be used to materialize any 64 bit scalar/vector splat by using two instances, one for the upper 32 bits and the other for the lower 32 bits. It should not materialize the cases which can be materialized by using the instruction xxspltidp.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Conanap requested review of this revision.Oct 26 2020, 10:49 AM
amyk added a subscriber: amyk.Oct 27 2020, 9:47 AM
amyk added inline comments.

Slightly over 80 characters.


These lines are also a bit over 80 characters, and I think we prefer to capitalize the variables.

nit: Maybe we can go with something like Hi and Lo?


nit: I believe the braces in all of these ifs can be omitted.


nit: Capitalize the sentence and end with period.


Might be a dumb question, but do you not need dl here?


Unintended change?


Since the CHECKs are usually generated by in this test case like stated on the top, I think the formatting of the CHECKs won't be outputted the way you currently have it the next time this test gets updated.

It might be better to just run the script and have all the CHECKs be separated out.

Conanap updated this revision to Diff 302886.Nov 4 2020, 10:11 AM
Conanap marked 5 inline comments as done.

Addressed some formatting problems as well as corrected incorrect arguments for Hi case.

Conanap marked an inline comment as done.Nov 4 2020, 10:11 AM

The unrelated change no longer shows as diff, thank you!

Conanap updated this revision to Diff 302894.Nov 4 2020, 10:20 AM
Conanap marked an inline comment as done.

Updated a test file.

Please clang-format the patch to pass pre-merge checks.

Conanap updated this revision to Diff 305568.Nov 16 2020, 11:52 AM


Conanap updated this revision to Diff 305575.Nov 16 2020, 12:13 PM

clang format

NeHuang requested changes to this revision.Nov 26 2020, 12:23 PM
NeHuang added inline comments.

please follow the naming convention in this file to use IsLE


Agree with Amy it is better to use Hi and Lo` as the variable names.
Also it is still taking more than 80 characters. Please double check if you properly clang-format it.


85 characters in this line, please double check you properly do clang-format


I do not quite understand the if and else logic here, current logic seems we can overwrite SplatNode after we execute SplatNode = DAG.getTargetConstant(0, dl, MVT::v2i64);
Is that as expected?


alignment issue


alignment issue


alignment issue

This revision now requires changes to proceed.Nov 26 2020, 12:23 PM
Conanap updated this revision to Diff 309039.Dec 2 2020, 1:07 PM
Conanap marked 6 inline comments as done.

Addressed some formatting comments

Replied to a comment


Thanks for bringing this up. SplatNode is reused in if (Lo) and if (Hi); it just saves a variable and a ternary statement (if (Hi) will also have to check if Lo created a SDNode before deciding between SplatNode or the SDNode created by Lo), but can understandably be confusing. I could separate them in to different variables and if statements to make it easier to understand if that's preferred.

amyk added inline comments.Dec 3 2020, 7:32 AM

Braces can be omitted here and on 9364 if it's just a single statement.


I think I'm still a little confused by this. Do we not need dl when we do getNode() here?


It would be good to put this under the "Anonymous Patterns" section.

addressed a comment


Hey so the DL is supplied from SplatNode's definition, and then we pass in SDLoc(SplatNode) here as the proper SDLoc after new instr def.

Conanap updated this revision to Diff 309534.Dec 4 2020, 7:20 AM
Conanap marked 4 inline comments as done.

Removed braces for single lines

Conanap added inline comments.Dec 7 2020, 3:20 PM

thanks for catching that, I meant to put it there originally.

nemanjai requested changes to this revision.Dec 11 2020, 7:06 AM

This is not functionally correct.


Nit: full sentences in comments. Capitalize the first word.


Won't this just produce a zero?


Initialize to undef to simplify the logic below.


The debug location comes from the original node (i.e. dl in this case). Just use that - no ternary operator.


Why does the index change depending on endianness? You are trying to produce a 64-bit pattern that is in no way dependent on endianness.

This revision now requires changes to proceed.Dec 11 2020, 7:06 AM
Conanap updated this revision to Diff 314431.Jan 4 2021, 12:17 PM
Conanap marked 4 inline comments as done.

Addressed some comments for formatting and style

Conanap marked an inline comment as done.Jan 4 2021, 12:28 PM

initialize as a form of undef instead

Conanap updated this revision to Diff 314439.Jan 4 2021, 12:41 PM

Removed unecessary ternary

Conanap updated this revision to Diff 314443.Jan 4 2021, 1:19 PM

Fixed a typo that prevented successful builds

NeHuang accepted this revision as: NeHuang.Jan 8 2021, 1:37 PM

LGTM and please wait for Nemanja's approval before committing this patch.

nemanjai requested changes to this revision.Jan 13 2021, 8:32 PM

This is still incorrect. The indices for the hi/low words are backwards. You can easily demonstrate this with a test case such as:

vector double test() { return (vector double)4.23422e12; }

#include <stdio.h>

vector double test();
int main(int argc, const char **argv) {
  vector double vec = test();
  return printf("{ %g, %g }\n", vec[0], vec[1]);

$ clang -O3 a.c b.c -mcpu=pwr10
$ ./a.out
{ 5.55224e+224, 5.55224e+224 }

The high word goes to index zero and the low word goes to index 1. This is backwards.


Why? Please don't use implicit conversion this way.
DAG.getTargetConstant() takes and SDLoc, you are passing an SDValue. This of course works because the former has a constructor that does the implicit conversion, but this is completely unnecessary and actually makes code less readable.

This revision now requires changes to proceed.Jan 13 2021, 8:32 PM
Conanap updated this revision to Diff 317355.Jan 18 2021, 7:55 AM
Conanap marked 2 inline comments as done.

Loads Hi before Lo now; removed implicit cast.

nemanjai accepted this revision.Jan 18 2021, 9:13 AM


This revision is now accepted and ready to land.Jan 18 2021, 9:13 AM
Conanap closed this revision.Jan 20 2021, 9:58 AM

Pushed; differential revision link accidentally had an extra https:// so it did not automatically close.