[PATCH] D90173: [PowerPC] Exploit splat instruction xxsplti32dx in Power10

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

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 }



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9405
+            DAG.getNode(PPCISD::XXSPLTI32DX, dl, MVT::v2i64, SplatNode,
+                        DAG.getTargetConstant(0, dl, MVT::i32),
+                        DAG.getTargetConstant(Lo, dl, MVT::i32));
----------------
The high word goes to index zero and the low word goes to index 1. This is backwards.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9366
+        SplatNode = DAG.getNode(
+            PPCISD::XXSPLTI32DX, SDLoc(SplatNode), MVT::v2i64, SplatNode,
+            DAG.getTargetConstant(isLE ? 1 : 0, SplatNode, MVT::i32),
----------------
Conanap wrote:
> amyk wrote:
> > I think I'm still a little confused by this. Do we not need `dl` when we do `getNode()` here?
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90173/new/

https://reviews.llvm.org/D90173