[Scalarizer] Avoid updating the name of globals

[bjope]

Summary

The "takeName" logic at the end of ScalarizerVisitor::finish
could end up renaming global variables when having simplified
and extractelement instruction to simply pick a single vector
element. If the input vector to the extractelement instruction
held pointers to global variables we ended up renaming the global
variable.
The patch make sure we only take the name of the replaced Op when
we have added new instructions that might need a useful name.

[lebedev.ri]

@bjope thanks! bugreport kinda skimmed my attention span, i was going to re-look today-ish.
I think this looks fine. The another alternative would be to only do that if the value-to-be-renamed is an instruction,
but then i'm not sure it's an improvement if we rename something that we reused.

[bjope]

In D86472#2234113, @lebedev.ri wrote:

@bjope thanks! bugreport kinda skimmed my attention span, i was going to re-look today-ish.
I think this looks fine. The another alternative would be to only do that if the value-to-be-renamed is an instruction,
but then i'm not sure it's an improvement if we rename something that we reused.

Yes, I actually experimented with adding an isa<Instruction> check first. But then I figured there could be more uses of the scalar value that is extraced from the vector, so changing the name of that value might be more confusing compared to keeping its name. So when comparing the result I preferred this way of solving it.

llvm/lib/Transforms/Scalar/Scalarizer.cpp

if (!Op->use_empty()) {
         BasicBlock *BB = Op->getParent();
         unsigned Count = cast<FixedVectorType>(Ty)->getNumElements();
         IRBuilder<> Builder(Op);
         if (isa<PHINode>(Op))
           Builder.SetInsertPoint(BB, BB->getFirstInsertionPt());
         for (unsigned I = 0; I < Count; ++I)
           Res = Builder.CreateInsertElement(Res, CV[I], Builder.getInt32(I),
                                             Op->getName() + ".upto" + Twine(I));
+        Res->takeName(Op);
       } else {
         assert(CV.size() == 1 && Op->getType() == CV[0]->getType());
         Res = CV[0];
         if (Op == Res)
           continue;
       }
-      Res->takeName(Op);
       Op->replaceAllUsesWith(Res);
     }
     PotentiallyDeadInstrs.emplace_back(Op);
   }
   Gathered.clear();
   Scattered.clear();
 
   RecursivelyDeleteTriviallyDeadInstructionsPermissive(PotentiallyDeadInstrs);

llvm/test/Transforms/Scalarizer/basic.ll

 ; CHECK: %res.i1 = call fast float @llvm.fma.f32
   %res = call fast <2 x float> @llvm.fma.v2f32(<2 x float> %x, <2 x float> %y, <2 x float> %z)
   ret <2 x float> %res
 }
 
 ; See https://reviews.llvm.org/D83101#2133062
 define <2 x i32> @f23_crash(<2 x i32> %srcvec, i32 %v1) {
 ; CHECK-LABEL: @f23_crash(
-; CHECK: %1 = extractelement <2 x i32> %srcvec, i32 0
-; CHECK: %t1.upto0 = insertelement <2 x i32> undef, i32 %1, i32 0
+; CHECK: %v0 = extractelement <2 x i32> %srcvec, i32 0
+; CHECK: %t1.upto0 = insertelement <2 x i32> undef, i32 %v0, i32 0
 ; CHECK: %t1 = insertelement <2 x i32> %t1.upto0, i32 %v1, i32 1
 ; CHECK: ret <2 x i32> %t1
   %v0 = extractelement <2 x i32> %srcvec, i32 0
   %t0 = insertelement <2 x i32> undef, i32 %v0, i32 0
   %t1 = insertelement <2 x i32> %t0, i32 %v1, i32 1
   ret <2 x i32> %t1
 }
 
 !0 = !{ !"root" }
 !1 = !{ !"set1", !0 }
 !2 = !{ !"set2", !0 }
 !3 = !{ !3, !{!"llvm.loop.parallel_accesses", !13} }
 !4 = !{ float 4.0 }
 !5 = !{ i64 0, i64 8, null }
 !13 = distinct !{}

llvm/test/Transforms/Scalarizer/global-bug.ll

+; RUN: opt < %s -scalarizer -S -o - | FileCheck %s
+; RUN: opt < %s -passes='function(scalarizer)' -S | FileCheck %s
+
+@a = dso_local global i16 0, align 1
+@b = dso_local local_unnamed_addr global i16 0, align 1
+
+; The scalarizer used to take the name of the extractelement instruction
+; ("extract") and put that on the extracted value, which in this test case is
+; the global variable @a. That was wrong, as we must not change the name of
+; the global variable. So make sure we find "@a" in the ptrtoint.
+define dso_local void @test1() local_unnamed_addr {
+; CHECK-LABEL: @test1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[T0:%.*]] = ptrtoint i16* @a to i16
+; CHECK-NEXT:    store i16 [[T0]], i16* @b, align 1
+; CHECK-NEXT:    ret void
+;
+entry:
+  %extract = extractelement <4 x i16*> <i16* @a, i16* @a, i16* @a, i16* @a>, i32 1
+  %t0 = ptrtoint i16* %extract to i16
+  store i16 %t0, i16* @b, align 1
+  ret void
+}