[SampleFDO] Make FSDiscriminator flag part of function parameters

[xur]

Summary

dd a parameter of IsFSDiscriminator to function
getBaseDiscriminator(), and
getBaseDiscriminatorFromDiscriminator().

Both functions currently check internal flag of
--enable-fs-discriminator. This is not good because we might
change the default value of the internal flag.

Note that we have a default parameter in
getBaseDiscriminatorFromDiscriminator(). This is just
because that the create_afdo_tool has a call-site to it.
I will remove the default parameter in a later patch.

This is pretty much NFC.

[xur]

Sent a wrong patch.
This is the patch I wanted to send for review.

[wmi]

llvm/include/llvm/IR/DebugInfoMetadata.h

2202–2205

Can we just use "unsigned DILocation::getBaseDiscriminator(bool IsFSDiscriminator = false)" or we still need the two API?

[xur]

llvm/include/llvm/IR/DebugInfoMetadata.h

2202–2205

Note that one still checks the option of enable-fs-discrminator. And the other uses a flag (when we know for sure the format).
If we just keep one, we will probably keep the one without parameters.

If we want to use the one with default (false) parameters, we need to set the options in many callers.

[wenlei]

lgtm.

llvm/include/llvm/IR/DebugInfoMetadata.h

2202–2205

This is indeed somewhat redundant. Understand that with two functions, we avoid changing many call sites, but it's probably cleaner to unify (default to false, pass EnableFSDiscriminator from callers when needed). I don't have a strong opinion.

llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h

284

This seems to be the most important change in the patch? :)

[wenlei]

llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h

284

Pasting email reply from Rong:

No. It's actually NFC in this patch. This is because this code is currently only called by ProfileLoader. In ProfileLoader, getDiscriminator() is the same as getBaseDiscriminator() because only base discriminator is used.
This only matters for FS-ProfileLoader.

Just to make sure I understand this correctly, this path will be shared between base/current profile loader and fs-profile loader, right?

[xur]

Integrated Wenlei's suggestion to remove the redundant interface.

[hoy]

LGTM.

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
   static unsigned getMaskedDiscriminator(unsigned D, unsigned B) {
     return (D & getN1Bits(B));
   }
 
   /// Return the bits used for base discriminators.
   static unsigned getBaseDiscriminatorBits() { return getBaseFSBitEnd(); }
 
   /// Returns the base discriminator for a given encoded discriminator \p D.
-  static unsigned getBaseDiscriminatorFromDiscriminator(unsigned D) {
-    if (EnableFSDiscriminator)
+  static unsigned
+  getBaseDiscriminatorFromDiscriminator(unsigned D,
+                                        bool IsFSDiscriminator = false) {
+    if (IsFSDiscriminator)
       return getMaskedDiscriminator(D, getBaseDiscriminatorBits());
     return getUnsignedFromPrefixEncoding(D);
   }
 
   /// Raw encoding of the discriminator. APIs such as cloneWithDuplicationFactor
   /// have certain special case behavior (e.g. treating empty duplication factor
   /// as the value '1').
   /// This API, in conjunction with cloneWithDiscriminator, may be used to encode
for (auto *LBF = dyn_cast<DILexicalBlockFile>(Scope);
        LBF = dyn_cast<DILexicalBlockFile>(Scope))
     Scope = LBF->getScope();
   DILexicalBlockFile *NewScope =
       DILexicalBlockFile::get(getContext(), Scope, getFile(), Discriminator);
   return DILocation::get(getContext(), getLine(), getColumn(), NewScope,
                          getInlinedAt());
 }
 
 unsigned DILocation::getBaseDiscriminator() const {
-  return getBaseDiscriminatorFromDiscriminator(getDiscriminator());
+  return getBaseDiscriminatorFromDiscriminator(getDiscriminator(),
+                                               EnableFSDiscriminator);
 }
 
 unsigned DILocation::getDuplicationFactor() const {
   return getDuplicationFactorFromDiscriminator(getDiscriminator());
 }
 
 unsigned DILocation::getCopyIdentifier() const {
   return getCopyIdentifierFromDiscriminator(getDiscriminator());
 }

llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h

if (!FS)
     return std::error_code();
 
   const DebugLoc &DLoc = Inst.getDebugLoc();
   if (!DLoc)
     return std::error_code();
 
   const DILocation *DIL = DLoc;
   uint32_t LineOffset = FunctionSamples::getOffset(DIL);
-  uint32_t Discriminator = DIL->getBaseDiscriminator();
+  uint32_t Discriminator;
+  if (EnableFSDiscriminator)
+    Discriminator = DIL->getDiscriminator();
+  else
+    Discriminator = DIL->getBaseDiscriminator();
+
   ErrorOr<uint64_t> R = FS->findSamplesAt(LineOffset, Discriminator);
   if (R) {
     bool FirstMark =
         CoverageTracker.markSamplesUsed(FS, LineOffset, Discriminator, R.get());
     if (FirstMark) {
       ORE->emit([&]() {
         OptRemarkAnalysisT Remark(DEBUG_TYPE, "AppliedSamples", &Inst);
         Remark << "Applied " << ore::NV("NumSamples", *R);
         Remark << " samples from profile (offset: ";
         Remark << ore::NV("LineOffset", LineOffset);
         if (Discriminator) {
           Remark << ".";
           Remark << ore::NV("Discriminator", Discriminator);
         }
         Remark << ")";
         return Remark;
       });
     }
-    LLVM_DEBUG(dbgs() << "    " << DLoc.getLine() << "."
-                      << DIL->getBaseDiscriminator() << ":" << Inst
-                      << " (line offset: " << LineOffset << "."
-                      << DIL->getBaseDiscriminator() << " - weight: " << R.get()
-                      << ")\n");
+    LLVM_DEBUG(dbgs() << "    " << DLoc.getLine() << "." << Discriminator << ":"
+                      << Inst << " (line offset: " << LineOffset << "."
+                      << Discriminator << " - weight: " << R.get() << ")\n");
   }
   return R;
 }
 
 /// Compute the weight of a basic block.
 ///
 /// The weight of basic block \p BB is the maximum weight of all the
 /// instructions in BB.

llvm/tools/llvm-profgen/ProfiledBinary.cpp

for (int32_t I = InlineStack.getNumberOfFrames() - 1; I >= 0; I--) {
     const auto &CallerFrame = InlineStack.getFrame(I);
     if (CallerFrame.FunctionName == "<invalid>")
       break;
     StringRef FunctionName(CallerFrame.FunctionName);
     if (UseCanonicalFnName)
       FunctionName = FunctionSamples::getCanonicalFnName(FunctionName);
     LineLocation Line(CallerFrame.Line - CallerFrame.StartLine,
                       DILocation::getBaseDiscriminatorFromDiscriminator(
-                          CallerFrame.Discriminator));
+                          CallerFrame.Discriminator,
+                          /* IsFSDiscriminator */ false));
     FrameLocation Callsite(FunctionName.str(), Line);
     CallStack.push_back(Callsite);
   }
 
   return CallStack;
 }
 
 InstructionPointer::InstructionPointer(ProfiledBinary *Binary, uint64_t Address,