[LV] Add analysis remark for mixed precision conversions

[jhuber6]

Summary

Floating point conversions inside vectorized loops have performance
implications but are very subtle. The user could specify a floating
point constant, or call a function without realizing that it will
force a change in the vector width. An example of this behaviour is
seen in https://godbolt.org/z/M3nT6c . The vectorizer should indicate
when this happens becuase it is most likely unintended behaviour.

This patch adds a simple check for this behaviour by following floating
point stores in the original loop and checking if a floating point
conversion operation occurs.

[jhuber6]

If anyone knows a better place to put this let me know. I couldn't find any existing infrastructure to run checks on the loop after its been vectorized.

[lebedev.ri]

This is already caught by -Wimplicit-float-conversion/-Wdouble-promotion though
https://godbolt.org/z/o5Pzh4

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9287

You should be checking the type of the value being stored, not the pointer's element type

[fhahn]

Please add test cases for the change.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9290

This potentially is a bit confusing, because a store has no uses (it's of type void). We are traversing the operands upwards.

9292

Can we collect all FP stores in a first pass, and then do the wordlist traversal for the all fp stores together afterwards? This way, we won't get duplicated remarks, if the same value is stored multiple times.

9636

Unrelated change?

9647

I think usually ORE->allowExtraAnalysis(LV_NAME); is used to check if extra analysis should be done for remarks.

[jdoerfert]

In D95539#2525648, @lebedev.ri wrote:

This is already caught by -Wimplicit-float-conversion/-Wdouble-promotion though
https://godbolt.org/z/o5Pzh4

Yes, but ;)

If you don't vectorize it is half as bad, arguably. Signal to noise might make the FE warnings unusable, every conversion is flagged after all https://godbolt.org/z/6hEjfx

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9292

I was expecting to have a set with all FPExt that have been diagnoses, but either way not duplicating for an FPExt seems reasonable.

[jdoerfert]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9324

Nits:

• make the function static.
• Visited doesn't need to be a map, use a SmallPtrSet and check like this: if (!Visited.insert(I).second) continue;

[jhuber6]

Making suggested changes. I'll add a test case later.

[jhuber6]

Adding test. Changing store type check.

[jhuber6]

ping

[fhahn]

Thanks for the update. I left a few comments with respect to the test case. I think it would be good to add a few more tests to cover all cases in the worklist loop, including using instructions defined outside the loop in the fpext and multiple stores that share a fpext.

llvm/test/Transforms/LoopVectorize/mixed-precision-remarks.ll

2 ↗

(On Diff #319676)

not needed

3 ↗

(On Diff #319676)

not needed

5 ↗

(On Diff #319676)

If this requires the x86_64 triple, it should go into the X86 subdirectory.

7 ↗

(On Diff #319676)

Personally I think the IR in the test case is concise and the C version should not be needed.

14 ↗

(On Diff #319676)

dso_local and local_unnamed_addr are not needed. Is #0 needed?

16 ↗

(On Diff #319676)

none of the instructions in the block should be needed?

33 ↗

(On Diff #319676)

!tbaa should not be needed here and at other places.

36 ↗

(On Diff #319676)

!llvm.loop should not be needed.

49 ↗

(On Diff #319676)

Could you try to prune the metadata a bit? I think we would only need a location for the instruction we generate a remark for.

[jhuber6]

Pruning test file metadata.

[fhahn]

In D95539#2566831, @jhuber6 wrote:

Pruning test file metadata.

Thanks, the test looks much better now! I think all that's missing is tests for the other cases mentioned earlier.

[jhuber6]

Adding test additional test case that checks for operands defined outside the
loop, and makes sure multiple remarks are not emitted for the same floating
point extension instruction.

[fhahn]

LGTM thanks! If you can, it might be good to add another test case where we emit multiple remarks.

llvm/test/Transforms/LoopVectorize/mixed-precision-remarks.ll

33 ↗

(On Diff #324370)

tiniest nit: now that we only branch to this block from for.body, it seems more naturally to have it at the end of the function :)

[jhuber6]

Adding an extra check for multiple codes. I'll land it later today.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

static bool processLoopInVPlanNativePath(
 
   // Mark the loop as already vectorized to avoid vectorizing again.
   Hints.setAlreadyVectorized();
 
   assert(!verifyFunction(*L->getHeader()->getParent(), &dbgs()));
   return true;
 }
 
+// Emit a remark if there are stores to floats that required a floating point
+// extension If the vectorized loop was generated with floating point there will
+// be a performance penalty from the conversion overhead and the change in the
+// vector width.
+void checkMixedPrecision(Loop *L, OptimizationRemarkEmitter *ORE) {
+  for (BasicBlock *BB : L->getBlocks()) {
+    for (Instruction &Inst : *BB) {
+      if (auto *S = dyn_cast<StoreInst>(&Inst)) {
+        if (!S->getPointerOperandType()->getPointerElementType()->isFloatTy())
+          continue;
+
+        // Traverse the uses of this floating point store inside the loop.
+        DenseMap<Instruction *, bool> Visited;
+        SmallVector<Instruction *, 8> Worklist;
+        Worklist.push_back(S);
+        while (!Worklist.empty()) {
+          auto *I = Worklist.pop_back_val();
+          if (!L->contains(I))
+            continue;
+          if (Visited[I])
+            continue;
+          Visited[I] = true;
+
+          // Emit a remark if the floating point store required a floating
+          // point conversion.
+          // TODO: More work could be done to identify the root cause such as a
+          // constant or a function return type and point the user to it.
+          if (isa<FPExtInst>(I))
+            ORE->emit([&]() {
+              return OptimizationRemarkAnalysis(LV_NAME, "VectorMixedPrecision",
+                                                I->getDebugLoc(),
+                                                L->getHeader())
+                     << "floating point conversion changes vector width. "
+                     << "Mixed floating point precision requires an up/down "
+                        "cast "
+                     << "that will negatively impact performance.";
+            });
+
+          for (Use &Op : I->operands())
+            if (auto *OpI = dyn_cast<Instruction>(Op))
+              Worklist.push_back(OpI);
+        }
+      }
+    }
+  }
+}
+
 LoopVectorizePass::LoopVectorizePass(LoopVectorizeOptions Opts)
     : InterleaveOnlyWhenForced(Opts.InterleaveOnlyWhenForced ||
                                !EnableLoopInterleaving),
       VectorizeOnlyWhenForced(Opts.VectorizeOnlyWhenForced ||
                               !EnableLoopVectorization) {}
 
 bool LoopVectorizePass::processLoop(Loop *L) {
   assert((EnableVPlanNativePath || L->isInnermost()) &&
if (EpilogueVF.Width.isVector()) {
 
       // Add metadata to disable runtime unrolling a scalar loop when there are
       // no runtime checks about strides and memory. A scalar loop that is
       // rarely used is not worth unrolling.
       if (!LB.areSafetyChecksAdded())
         DisableRuntimeUnroll = true;
     }
 
+    // runtime checks that get used.
     // Report the vectorization decision.
     ORE->emit([&]() {
       return OptimizationRemark(LV_NAME, "Vectorized", L->getStartLoc(),
                                 L->getHeader())
              << "vectorized loop (vectorization width: "
              << NV("VectorizationFactor", VF.Width)
              << ", interleaved count: " << NV("InterleaveCount", IC) << ")";
     });
+
+    auto &Ctx = L->getHeader()->getModule()->getContext();
+    if (Ctx.getDiagHandlerPtr()->isAnyRemarkEnabled(LV_NAME))
+      checkMixedPrecision(L, ORE);
   }
 
   Optional<MDNode *> RemainderLoopID =
       makeFollowupLoopID(OrigLoopID, {LLVMLoopVectorizeFollowupAll,
                                       LLVMLoopVectorizeFollowupEpilogue});
   if (RemainderLoopID.hasValue()) {
     L->setLoopID(RemainderLoopID.getValue());
   } else {