[PATCH] D57857: [PowerPC] custom lower `v2f64 fpext v2f32`

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think approaching the problem this way is unnecessarily limited - and I say that knowing that I may have suggested a similar approach.
However, what we are actually looking to do is to convert a DAG such as:

  t1: v2f32,ch = load(...)
  t2: v2f32,ch = load(...)
  # arbitrary operations on v2f32
  tN: v2f64 = fp_extend...

into:

  t1: v2f32,ch = load(...)
  t2: v2f32,ch = load(...)
  t3: v2f64 = fp_extend t1
  t4: v2f64 = fp_extend t2
  # widen all operations to v2f64

And then all we need is a custom load of two `float` values into a vector of two `double` values. I think the best way to do that would be to combine any occurrences of
`(v2f64 fp_extend (v2f32 op (v2f32 op_input1), (v2f32 op_input2)...))` (i.e. as long as all its inputs are of type `v2f32`)
into
`(v2f64 op (fp_extend op_input1) [, (fp_extend op_input2)...])`



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9524
+
+  // return value is not MTV::v2f64 or param is not v2f32
+  if (Op.getValueType() != MVT::v2f64 ||
----------------
`// We only want to custom lower an extend from v2f32 to v2f64.`

Talking about return values or parameters on SD Nodes seems quite unnatural.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9544
+        return SDValue();
+      // Generate new load DAG.
+      LoadSDNode *LD = cast<LoadSDNode>(LdOp);
----------------
You are not generating a new DAG, just a new node.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9568
+  }
+  llvm_unreachable("Should never reach here!");
+}
----------------
It is obvious that an `llvm_unreachable` should not be reached. A more descriptive comment is desired. This is equivalent to emitting a message when the compiler crashes to say "Compiler crashed".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:408
+      /// Custom extend v4f32 to v2f64.
+      FP_EXTEND_LHW,
+
----------------
I don't think you should use the abbreviation `LHW` in these as that seems to suggest a "half-word" and that is not the case. This is the "low half" of a VSR (which is itself a doubleword). I believe `FP-EXTEND_LH` and `LD_VSX_LH` should be adequate (the name of the node as you have it makes it seem as if it mimics an ISA mnemonic, which it does not).


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

https://reviews.llvm.org/D57857