[xray] Add xray-ignore-loops option

[ianlevesque]

Summary

XRay allows tuning by minimum function size, but also always instruments
functions with loops in them. If the minimum function size is set to a
large value the loop instrumention ends up causing most functions to be
instrumented anyway. This adds a new flag, xray-ignore-loops, to disable
the loop detection logic.

[ianlevesque]

Remove extraneous changes

[smeenai]

To make sure I'm understanding this correctly, previously, for any function with a loop in it, XRay would ignore the minimum instruction count threshold and instrument it unconditionally. You're adding an option to make XRay respect the threshold even if the function has loops. Is that correct?

I'm wondering what the reason is for the existing logic ignoring loops, and if we can incorporate loops into the existing logic instead of adding another option (and whether that would be better). I'm sure @dberris can speak more to that, so leaving this to him.

[dberris]

LGTM

I'm wondering how you're imbuing these attributes to the functions from the front-ends -- is this something you intend to support as a flag in Clang for example?

In D72659#1818503, @smeenai wrote:

To make sure I'm understanding this correctly, previously, for any function with a loop in it, XRay would ignore the minimum instruction count threshold and instrument it unconditionally. You're adding an option to make XRay respect the threshold even if the function has loops. Is that correct?

I'm wondering what the reason is for the existing logic ignoring loops, and if we can incorporate loops into the existing logic instead of adding another option (and whether that would be better). I'm sure @dberris can speak more to that, so leaving this to him.

Because we assume that loops that haven't been optimised out might have variable runtime, we ensure that any code that has loops might have interesting performance characteristics and always instrument those. That might have been a conservative assumption, and it might be a bit hard to manually mark functions that must never be instrumented instead, so I understand the need for something like this attribute to be more targeted.

[ianlevesque]

In D72659#1818816, @dberris wrote:

I'm wondering how you're imbuing these attributes to the functions from the front-ends -- is this something you intend to support as a flag in Clang for example?

Yes exactly. I will have the Clang diff up shortly.

llvm/lib/CodeGen/XRayInstrumentation.cpp

 }
 
 bool XRayInstrumentation::runOnMachineFunction(MachineFunction &MF) {
   auto &F = MF.getFunction();
   auto InstrAttr = F.getFnAttribute("function-instrument");
   bool AlwaysInstrument = !InstrAttr.hasAttribute(Attribute::None) &&
                           InstrAttr.isStringAttribute() &&
                           InstrAttr.getValueAsString() == "xray-always";
-  Attribute Attr = F.getFnAttribute("xray-instruction-threshold");
-  unsigned XRayThreshold = 0;
+  auto ThresholdAttr = F.getFnAttribute("xray-instruction-threshold");
+  auto IgnoreLoopsAttr = F.getFnAttribute("xray-ignore-loops");
+  unsigned int XRayThreshold = 0;
   if (!AlwaysInstrument) {
-    if (Attr.hasAttribute(Attribute::None) || !Attr.isStringAttribute())
+    if (ThresholdAttr.hasAttribute(Attribute::None) ||
+        !ThresholdAttr.isStringAttribute())
       return false; // XRay threshold attribute not found.
-    if (Attr.getValueAsString().getAsInteger(10, XRayThreshold))
+    if (ThresholdAttr.getValueAsString().getAsInteger(10, XRayThreshold))
       return false; // Invalid value for threshold.
 
+    bool IgnoreLoops = !IgnoreLoopsAttr.hasAttribute(Attribute::None);
+
     // Count the number of MachineInstr`s in MachineFunction
     int64_t MICount = 0;
     for (const auto &MBB : MF)
       MICount += MBB.size();
 
+    bool TooFewInstrs = MICount < XRayThreshold;
+
+    if (!IgnoreLoops) {
       // Get MachineDominatorTree or compute it on the fly if it's unavailable
       auto *MDT = getAnalysisIfAvailable<MachineDominatorTree>();
       MachineDominatorTree ComputedMDT;
       if (!MDT) {
         ComputedMDT.getBase().recalculate(MF);
         MDT = &ComputedMDT;
       }
 
       // Get MachineLoopInfo or compute it on the fly if it's unavailable
       auto *MLI = getAnalysisIfAvailable<MachineLoopInfo>();
       MachineLoopInfo ComputedMLI;
       if (!MLI) {
         ComputedMLI.getBase().analyze(MDT->getBase());
         MLI = &ComputedMLI;
       }
 
       // Check if we have a loop.
       // FIXME: Maybe make this smarter, and see whether the loops are dependent
       // on inputs or side-effects?
-    if (MLI->empty() && MICount < XRayThreshold)
+      if (MLI->empty() && TooFewInstrs)
         return false; // Function is too small and has no loops.
+    } else if (TooFewInstrs) {
+      // Function is too small
+      return false;
+    }
   }
 
   // We look for the first non-empty MachineBasicBlock, so that we can insert
   // the function instrumentation in the appropriate place.
   auto MBI = llvm::find_if(
       MF, [&](const MachineBasicBlock &MBB) { return !MBB.empty(); });
   if (MBI == MF.end())
     return false; // The function is empty.

llvm/test/CodeGen/X86/xray-ignore-loop-detection.ll

 ; RUN: llc -verify-machineinstrs -filetype=asm -o - -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -filetype=asm -o - -mtriple=x86_64-darwin-unknown    < %s | FileCheck %s
 
-define i32 @foo(i32 %i) nounwind noinline uwtable "xray-instruction-threshold"="1" {
+define i32 @foo(i32 %i) nounwind noinline uwtable "xray-instruction-threshold"="10" "xray-ignore-loops" {
 entry:
   br label %Test
 Test:
   %indvar = phi i32 [0, %entry], [%nextindvar, %Inc]
   %cond = icmp eq i32 %indvar, %i
   br i1 %cond, label %Exit, label %Inc
 Inc:
   %nextindvar = add i32 %indvar, 1
   br label %Test
 Exit:
   %retval = phi i32 [%indvar, %Test]
   ret i32 %retval
 }
 
-; CHECK-LABEL: xray_sled_0:
-; CHECK-NEXT:  .ascii "\353\t"
-; CHECK-NEXT:  nopw 512(%rax,%rax)
-
+; CHECK-NOT: xray_sled_0:

llvm/test/CodeGen/X86/xray-loop-detection.ll

 ; RUN: llc -verify-machineinstrs -filetype=asm -o - -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -filetype=asm -o - -mtriple=x86_64-darwin-unknown    < %s | FileCheck %s
 
-define i32 @foo(i32 %i) nounwind noinline uwtable "xray-instruction-threshold"="1" {
+define i32 @foo(i32 %i) nounwind noinline uwtable "xray-instruction-threshold"="10" {
 entry:
   br label %Test
 Test:
   %indvar = phi i32 [0, %entry], [%nextindvar, %Inc]
   %cond = icmp eq i32 %indvar, %i
   br i1 %cond, label %Exit, label %Inc
 Inc:
   %nextindvar = add i32 %indvar, 1