[PATCH] D54663: [PowerPC] Complete the custom legalization of vector int to fp conversion

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 20 18:35:40 PST 2018


nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7333
+    Arrange = DAG.getBitcast(IntermediateVT, Arrange);
+    Extend = DAG.getNode(ISD::SIGN_EXTEND_INREG, dl, IntermediateVT, Arrange,
+                         DAG.getValueType(Op.getOperand(0).getValueType()));
----------------
RolandF wrote:
> When I looked at this before, I found that using SIGN_EXTEND_INREG led to a perm/shift left/shift right pattern, and I thought that using unpack would be faster for the signed int/pwr8 case.
Ah cool, I didn't think of that. You're right an unpack is definitely useful for something like this (unpack low for LE and high for BE).
However, I think that would be good to do irrespective of this and in a follow-up patch.
What I'm getting at is that however the pertinent pattern such as:
`sign_extend_inreg (vector_shuffle ...)`
came into existence, it's good to emit optimal code for it.


================
Comment at: test/CodeGen/PowerPC/vec_conv_i16_to_fp32_elts.ll:83
 ; CHECK-P8:       # %bb.0: # %entry
+; CHECK-P8-NEXT:    addis r4, r2, .LCPI1_0 at toc@ha
 ; CHECK-P8-NEXT:    mtvsrd f0, r3
----------------
jsji wrote:
> Shall we check the data in .LCPI1_0 to make sure the permute control is correct?
I think that doing it for all of them would be time consuming without a particularly good justification. Furthermore, these check patterns were produced by the script so if anyone updates anything about the codegen in the future and re-runs the script, those checks would be lost. Perhaps I could do it for one only, but that seems rather arbitrary.

If you insist, I'll certainly add it.


================
Comment at: test/CodeGen/PowerPC/vsx.ll:1096
 
 ; This gets scalarized so the code isn't great
 define <2 x double> @test69(<2 x i16> %a) {
----------------
jsji wrote:
> Should we remove this comment now?
Good point. I'll do that.


Repository:
  rL LLVM

https://reviews.llvm.org/D54663





More information about the llvm-commits mailing list