[PATCH] D43774: [InstCombine] Add constant vector support to lookThroughFPExtensions for visitFPTrunc.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1426-1428
+  // See if the value can be truncated to half and then reextended.
+  if (Constant *NewCFP = fitsInFPType(CFP, APFloat::IEEEhalf()))
+    return NewCFP;
----------------
The existing code has a bug, but I don't think there's a way to expose it other than as an inefficiency. We will (usually?) create a half constant and then extend it back to single because that's not the type we wanted in the first place. 

For scalars, this is wasteful, but constant folding hides the problem. Maybe nobody cares for scalars, but now we're doing that extra work N times for vector constants. And the bug is now visible (so we should add a test like this where the constants can't shrink to the same narrow type):

```

define <2 x float> @not_half_shrinkable(<2 x float> %x) {
  %ext = fpext <2 x float> %x to <2 x double>
  %add = fadd <2 x double> %ext, <double 0.0, double 2049.0>
  %r = fptrunc <2 x double> %add to <2 x float>
  ret <2 x float>  %r
}

```


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1471-1475
+  // Try to do the same for vectors of FP constants.
+  if (auto *CV = dyn_cast<Constant>(V))
+    if (CV->getType()->isVectorTy())
+      if (Constant *NewCV = shrinkFPConstantVector(CV))
+        return NewCV;
----------------
The usual pattern has the vector constant check inside the helper function, so move the vector type check into 'shrinkFPConstantVector' and rename the functions?

Ideally, these wouldn't be instcombine-specific helpers. They'd be queries on constants. I recently made a similar change in rL325398.


================
Comment at: test/Transforms/InstCombine/fpextend.ll:92-96
   %tmp = load <2 x float>, <2 x float>* @Z, align 4
   %tmp1 = fpext <2 x float> %tmp to <2 x double>
   %tmp3 = fadd <2 x double> %tmp1, <double 0.000000e+00, double 0.000000e+00>
   %tmp34 = fptrunc <2 x double> %tmp3 to <2 x float>
   store <2 x float> %tmp34, <2 x float>* @Z, align 4
----------------
I think we can remove the load/store/globals from all of the tests in this file as a preliminary cleanup.


Repository:
  rL LLVM

https://reviews.llvm.org/D43774