[PATCH] D55681: [llvm] API for encoding/decoding DWARF discriminators.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Not sure I'm following this all in detail & at least for me (someone not super familiar with the details here) it might've been a bit easier to review in smaller pieces (breaking up the refactoring to generalize the components of the discriminator into smaller bits, etc) but all good. Thanks!



================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2057-2058
+  unsigned CI = getCopyIdentifier();
+  Optional<unsigned> D = encodeDiscriminator(BD, DF, CI);
+  if (D)
+    return cloneWithDiscriminator(D.getValue());
----------------
You could roll the variable declaration into the condition:

  if (Optional<unsigned> D = ...)


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2059
+  if (D)
+    return cloneWithDiscriminator(D.getValue());
+  return None;
----------------
If you like, you can use *D instead of D.getValue()


================
Comment at: lib/IR/DebugInfoMetadata.cpp:152-155
+  BD = getUnsignedFromPrefixEncoding(D);
+  DF = getUnsignedFromPrefixEncoding(getNextComponentInDiscriminator(D));
+  CI = getUnsignedFromPrefixEncoding(
+      getNextComponentInDiscriminator(getNextComponentInDiscriminator(D)));
----------------
Perhaps these should call getBaseDiscriminatorFromDiscriminator, getDuplicationFactorFromDiscriminator, getCopyIdentifierFromDiscriminator?

(though it looks like getDuplicationFactorFromdiscriminator has some extra logic that isn't in the initialization for DF here in decodeDiscriminator - is that a bug that these two cases are handled differently?)


================
Comment at: lib/Target/X86/X86DiscriminateMemOps.cpp:138
         Changed = true;
         const std::pair<DenseSet<unsigned>::iterator, bool> MustInsert =
             Set.insert(DI->getBaseDiscriminator());
----------------
I'd probably drop the top level 'const' here (since it's a value rather than a reference - LLVM doesn't usually use top level const like this)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55681