[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

[Rong Xu via Phabricator via llvm-commits]

xur marked 4 inline comments as done.
xur added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2229-2230
     return this;
+  if (EnableFSDiscriminator)
+    return cloneWithDiscriminator(D);
   if (Optional<unsigned> Encoded = encodeDiscriminator(D, DF, CI))
----------------
wmi wrote:
> EnableFSDiscriminator has been handled above already?
Good catch. This is from a bad merge. Fixed.


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:165
+template <>
+void SampleProfileLoaderBaseImpl<MachineBasicBlock>::buildEdges(FunctionT &F) {
+  for (auto &BI : F) {
----------------
wmi wrote:
> What is its difference with the generic version of SampleProfileLoaderBaseImpl<BT>::buildEdges? They look similar.
There are basically the same. We have this just because IR and MIR have different interfaces for predecessors and successors. In IR, there is a utility iteration while MIR's iterators are with CFG.
I could add utilities in MIR to remove this specialization.  


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:242
+          continue;
+        BB->setSuccProbability(SI, NewProb);
+        bool Show = false;
----------------
wmi wrote:
> Looks like the BB prob update shouldn't be guarded by ShowFSBranchProb.
Yes. that's correct. I added this option just before sending the review to suppress the debug output. It's not added correctly. fixed


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:276
+  auto ReaderOrErr =
+      sampleprof::SampleProfileReader::create(Filename, Ctx, P, "");
+  if (std::error_code EC = ReaderOrErr.getError()) {
----------------
wmi wrote:
> We may want to set remapping file otherwise remapping cannot be effective enough when FSAFDO is enabled.
OK. Seems that I need to pass the remap file name the same way as profile file name. Will fix.


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

https://reviews.llvm.org/D107878