[PATCH] D143593: [InstCombine] Don't fold freeze poison when it's used in shufflevector

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D143593#4118517 <https://reviews.llvm.org/D143593#4118517>, @ManuelJBrito wrote:

> Alternatively I was thinking of moving this match into getUndefReplacement and let it be a case where the undef replacement does no replacing.
> After reading the discussion here [ https://reviews.llvm.org/D123962 ], I think this approach makes the change cleaner and conceptually more sound.
> Would this make sense?

Not sure if that's worth the effort - you'd have to adjust the caller too to allow no replacement?
It looks like the updated patch revision is on top of the previous revision rather than `main`, so that needs to be fixed.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4019-4021
+    // Don't fold freeze(undef/poison) if it's used has the second vector in a shuffle.
+    // This is necessary for the backend to generate efficient assembly for
+    // mm*_cast* intel intrinsics.
----------------
The indentation is off, and we don't want to mention any x86-specific hackery here. 
Just say something like:
  // This may improve codegen for shuffles that allow unspecified inputs.


================
Comment at: llvm/test/Transforms/InstCombine/shufflevector_freezepoison.ll:3
+
+define <4 x double> @shuffle_op1_freeze_poison(<2 x double> %a) {
+; CHECK-LABEL: @shuffle_op1_freeze_poison(
----------------
This isn't what I was expecting. If we're bailing out for operand 1, then shouldn't we be symmetric and do the same for operand 0 (just in case that logically equivalent pattern was somehow created in the front-end)?


================
Comment at: llvm/test/Transforms/InstCombine/shufflevector_freezepoison.ll:9
+  %b = freeze <2 x double> poison
+  call void @use(<2 x double> %b)
+  %shuffle = shufflevector <2 x double> %b, <2 x double> %a, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
----------------
Why do all of the tests have an extra use? If we don't expect the behavior to change with extra uses, then it would be better if some tests have the extra use and some do not.


================
Comment at: llvm/test/Transforms/InstCombine/shufflevector_freezepoison.ll:18
+; CHECK-NEXT:   call void @use(<2 x double> [[FR]])
+; CHECK-NEXT:   [[SV:%.*]] = shufflevector <2 x double> %{{.*}}, <2 x double> [[FR]], <4 x i32> <i32 0, i32 1, i32 2, i32 3>
+; CHECK-NEXT:   ret <4 x double> [[SV]]
----------------
Are the CHECK lines manually written? Use utils/update_test_checks.py to auto-generate those lines instead.
You can pre-commit this test file to main with the baseline CHECKs, so we'll just see diffs in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143593