[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

2203–2210

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

[xur]

llvm/include/llvm/IR/DebugInfoMetadata.h

2203–2210

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

2203–2210

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:
   /// duplication factor (or 0) is encoded.
   inline unsigned getDuplicationFactor() const;
 
   /// Returns the copy identifier stored in the discriminator.
   inline unsigned getCopyIdentifier() const;
 
   /// Returns the base discriminator stored in the discriminator.
   inline unsigned getBaseDiscriminator() const;
+  inline unsigned getBaseDiscriminator(bool IsFSDiscriminator) const;
 
   /// Returns a new DILocation with duplication factor \p DF * current
   /// duplication factor encoded in the discriminator. The current duplication
   /// factor is as defined by getDuplicationFactor().
   /// Returns None if encoding failed.
   inline Optional<const DILocation *> cloneByMultiplyingDuplicationFactor(unsigned DF) const;
 
   /// When two instructions are combined into a single instruction we also
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::getBaseDiscriminator(bool IsFSDiscriminator) const {
+  return getBaseDiscriminatorFromDiscriminator(getDiscriminator(),
+                                               IsFSDiscriminator);
 }
 
 unsigned DILocation::getDuplicationFactor() const {
   return getDuplicationFactorFromDiscriminator(getDiscriminator());
 }
 
 unsigned DILocation::getCopyIdentifier() const {
   return getCopyIdentifierFromDiscriminator(getDiscriminator());
 }
 
 Optional<const DILocation *> DILocation::cloneWithBaseDiscriminator(unsigned D) const {
   unsigned BD, DF, CI;
 
   if (EnableFSDiscriminator) {
-    BD = getBaseDiscriminator();
+    BD = getBaseDiscriminator(/* IsFSDiscriminator */ true);
     if (D == BD)
       return this;
     return cloneWithDiscriminator(D);
   }
 
   decodeDiscriminator(getDiscriminator(), BD, DF, CI);
   if (D == BD)
     return this;
   if (Optional<unsigned> Encoded = encodeDiscriminator(D, DF, CI))
     return cloneWithDiscriminator(*Encoded);
   return None;
 }
 
 Optional<const DILocation *> DILocation::cloneByMultiplyingDuplicationFactor(unsigned DF) const {
   assert(!EnableFSDiscriminator && "FSDiscriminator should not call this.");
 
   DF *= getDuplicationFactor();
   if (DF <= 1)
     return this;
 
-  unsigned BD = getBaseDiscriminator();
+  unsigned BD = getBaseDiscriminator(/* IsFSDiscriminator */ false);
   unsigned CI = getCopyIdentifier();
   if (Optional<unsigned> D = encodeDiscriminator(BD, DF, CI))
     return cloneWithDiscriminator(*D);
   return None;
 }
 
 class DINamespace : public DIScope {
   friend class LLVMContextImpl;

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(/* IsFSDiscriminator */ false);
+
   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,