This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Exploit splat instruction xxsplti32dx in Power10
ClosedPublic

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

Details

Reviewers
nemanjai
saghir
NeHuang
Group Reviewers
Restricted Project
Summary

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

Conanap created this revision.Oct 26 2020, 10:49 AM
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.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9187

Slightly over 80 characters.

9196

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?

9200

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

9201

nit: Capitalize the sentence and end with period.

9212

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

9226–9227

Unintended change?

llvm/test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll
17–18

Since the CHECKs are usually generated by update_llc_test_checks.py 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

Clang-format

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.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9181

please follow the naming convention in this file to use IsLE

9196

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.

9197

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

9205

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?

llvm/test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll
17–18

alignment issue

19

alignment issue

52–53

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

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9205

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
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9207

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

9216

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

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2355

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

addressed a comment

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9216

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
llvm/lib/Target/PowerPC/PPCInstrPrefix.td
2355

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.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9195

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

9199

Won't this just produce a zero?

9201

Initialize to undef to simplify the logic below.

9209

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

9211

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:

a.c:
vector double test() { return (vector double)4.23422e12; }

b.c:
#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 }
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9210

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

9216

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

LGTM.

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.