[PATCH] D151419: [DebugInfo][RemoveDIs] Eliminate some debug-intrinsics-affect-codegen errors

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

> Slight difference -- all the changes in this specific patch are for DebugLocs, to avoid "real" instructions setting their locations from debug-instructions. That legitimately will be designed away,

That's not what the patch description says; it's about eliminating codegen errors.  Are you mixing two patches into one? I've commented on the ones that look like codegen fixes.

Now, about testing... like @dblaikie I'm not very comfortable with this no-testing business, although I see he accepted the patch already. Are the DebugLoc changes in fact NFC? Taking the DebugLoc from non-debug instructions changes nothing? I know we have next to no tests for this sort of thing, so maybe there's no immediate value in generating a bunch, but I wanted to know whether the intent is just to prep for eliminating debug instructions, or actually making things better in the source-location aspect.

As for the gnochange bits, those are functional changes and we will not reach the Promised Land of no debug instructions for at least one additional release. Given past contentiousness over IR transform tests and debug info, I'd feel a whole lot better if we had tests--with checks generated by the scripts that the IR-transform mavens want to use--to make sure we don't regress in the meantime.



================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1889
   BasicBlock *LoopBody = *(CurLoop->block_begin());
-  if (LoopBody->size() >= 20) {
+  if (LoopBody->sizeWithoutDebug() >= 20) {
     // The loop is too big, bail out.
----------------
Looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5351
   while (Rewriter.isInsertedInstruction(&*IP) && IP != LowestIP)
-    ++IP;
+    IP = IP->getNextNonDebugInstruction()->getIterator();;
 
----------------
Double semicolon at the end.
Is this strictly a performance improvement? (in which case possibly the "Ignore debug intrinsics" loop a few lines above can go away?)
Or if we find a debug instruction does this prematurely break out of the loop (making it a gnochange bug)? I guess it depends on how `isInsertedInstruction` responds; I don't mess around inside optimizations enough to know.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:102
+  BasicBlock::iterator IP = std::next(I->getIterator());
+  IP = skipDebugIntrinsics(IP);
   if (auto *II = dyn_cast<InvokeInst>(I))
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:150
          "Expected the cast argument to be a global/constant");
-  return Builder.GetInsertBlock()
+  return skipDebugIntrinsics(Builder.GetInsertBlock()
       ->getParent()
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3391
 
-      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode()) {
+      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
         auto *SD = getScheduleData(I);
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3424
     void initialFillReadyList(ReadyListType &ReadyList) {
-      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode()) {
+      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
         doForAllOpcodes(I, [&](ScheduleData *SD) {
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11139
     if (ScheduleEnd != OldScheduleEnd) {
-      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode())
+      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction())
         doForAllOpcodes(I, [](ScheduleData *SD) { SD->clearDependencies(); });
----------------
Another gnochange fix?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11287
     ScheduleStart = I;
-    ScheduleEnd = I->getNextNode();
+    ScheduleEnd = I->getNextNonDebugInstruction();
     if (isOneOf(S, I) != I)
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11329
                    nullptr);
-  ScheduleEnd = I->getNextNode();
+  ScheduleEnd = I->getNextNonDebugInstruction();
   if (isOneOf(S, I) != I)
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11342
   ScheduleData *CurrentLoadStore = PrevLoadStore;
-  for (Instruction *I = FromI; I != ToI; I = I->getNextNode()) {
+  for (Instruction *I = FromI; I != ToI; I = I->getNextNonDebugInstruction()) {
     // No need to allocate data for non-schedulable instructions.
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11443
+        for (Instruction *I = BundleMember->Inst->getNextNonDebugInstruction();
+             I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
           if (isSafeToSpeculativelyExecute(I, &*BB->begin(), SLP->AC))
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11463
+          for (Instruction *I = BundleMember->Inst->getNextNonDebugInstruction();
+               I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
             if (match(I, m_Intrinsic<Intrinsic::stacksave>()) ||
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11486
+          for (Instruction *I = BundleMember->Inst->getNextNonDebugInstruction();
+               I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
             if (!match(I, m_Intrinsic<Intrinsic::stacksave>()) &&
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11570
          "tried to reset schedule on block which has not been scheduled");
-  for (Instruction *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode()) {
+  for (Instruction *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
     doForAllOpcodes(I, [&](ScheduleData *SD) {
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11610
   for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd;
-       I = I->getNextNode()) {
+       I = I->getNextNonDebugInstruction()) {
     BS->doForAllOpcodes(I, [this, &Idx, BS](ScheduleData *SD) {
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11638
       Instruction *pickedInst = BundleMember->Inst;
-      if (pickedInst->getNextNode() != LastScheduledInst)
+      if (pickedInst->getNextNonDebugInstruction() != LastScheduledInst)
         pickedInst->moveBefore(LastScheduledInst);
----------------
This looks like a gnochange fix.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:11653
   // Check that all schedulable entities got scheduled
-  for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd; I = I->getNextNode()) {
+  for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd; I = I->getNextNonDebugInstruction()) {
     BS->doForAllOpcodes(I, [&](ScheduleData *SD) {
----------------
This looks like a gnochange fix.


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

https://reviews.llvm.org/D151419