[PATCH] D76983: [InstCombine] Transform extractelement-trunc -> bitcast-extractelement

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

See inline for a few code nits, and I'd make some test changes:

1. Combine the 2 separate test files into 1 to reduce duplication and make the endian diffs easier to see. Take a look at llvm/test/Transforms/InstCombine/extractelement.ll to see an example.
2. Name the file based on the transform rather than a bug name.
3. Add a test that corresponds to the larger pattern from the bug report, so we know that the sequence of transforms within instcombine works on the motivating bug:

  define <4 x i64> @PR45314(<4 x i64> %x) {
    %e = extractelement <4 x i64> %x, i32 0
    %t = trunc i64 %e to i32
    %i = insertelement <8 x i32> undef, i32 %t, i32 0
    %s = shufflevector <8 x i32> %i, <8 x i32> undef, <8 x i32> zeroinitializer
    %b = bitcast <8 x i32> %s to <4 x i64>
    ret <4 x i64> %b
  }



4. Generate the baseline CHECK lines without this code patch in place and push the tests to master as a preliminary NFC patch. Then apply this code patch, so we see the code diffs resulting from this patch in this review. That way, the tests will safely remain even if this code patch has to be reverted for some reason.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:847
 
+  /// Whenever an element is extracted from a vector, and then truncated,
+  /// canonicalize by converting it to a bitcast followed by an
----------------
I don't think we use the triple-slash documentation comment within a function. Either change to the standard "//" or make a helper function with the doxygen comment (similar to "foldVecTruncToExtElt" just above here).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:855
+  ///   extractelement <8 x i32> (bitcast <4 x i64> %X to <8 x i32>), i32 0
+  Value *VecOp = nullptr;
+  if (match(Src,
----------------
Don't initialize this to nullptr. If the match fails, the variable should not be used, so initializing hides a potential compile-time warning for an unintended use of that variable. The same should be true of the existing variables, so if you want to fix them 1st with an NFC patch, that would be ok.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:864
+    // A badly fit destination size would result in an invalid cast.
+    if (VecNumElts * VecOpScalarSize % DestScalarSize == 0) {
+      unsigned BitCastNumElts = VecNumElts * VecOpScalarSize / DestScalarSize;
----------------
The VecNumElts factor doesn't change the modulo constraint?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:865
+    if (VecNumElts * VecOpScalarSize % DestScalarSize == 0) {
+      unsigned BitCastNumElts = VecNumElts * VecOpScalarSize / DestScalarSize;
+      unsigned VecOpIdx = Cst->getZExtValue();
----------------
Would be slightly easier to read if we made a local name for the common factor like:
  unsigned TruncRatio = VecOpScalarSize / DestScalarSize;


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

https://reviews.llvm.org/D76983