[PseudoProbe] Remove unnecessary asserts about non-zero discriminator.

[hoy]

Summary

Despite previous efforts in fixing accidentally setting deduplication factor and avoiding enforcing a callsite debug loc for pseudo probes, I'm still seeing an IR probe having a non-zero discriminator. This time it is due to the merge of two probes with irreconsilable debug locations and the merged probe ends up getting the original callsite locs. Therefore I'm removing the assert about IR probe should always have a zero discriminator. This safe since

• Probe discriminators are only emitted in FS-AFDO mode; and
• The first FS discriminator assigning pass always clears non-discriminators left over from IR passes.

[wenlei]

In general if the effort to make sure probe has zero discriminator is too much, it makes sense to just rely on discriminator clean up instead during FS discriminator assignment for probes. I'm curious though what exposed this case? IIRC, we didn't hit this assert for a while now.

[hoy]

In D155252#4499632, @wenlei wrote:

In general if the effort to make sure probe has zero discriminator is too much, it makes sense to just rely on discriminator clean up instead during FS discriminator assignment for probes. I'm curious though what exposed this case? IIRC, we didn't hit this assert for a while now.

Yeah, it was good for a while until incrPGO was on. I suspect incrPGO opened more opportunity for optimizations and exposed this issue.

[wenlei]

In D155252#4501305, @hoy wrote:

In D155252#4499632, @wenlei wrote:

In general if the effort to make sure probe has zero discriminator is too much, it makes sense to just rely on discriminator clean up instead during FS discriminator assignment for probes. I'm curious though what exposed this case? IIRC, we didn't hit this assert for a while now.

Yeah, it was good for a while until incrPGO was on. I suspect incrPGO opened more opportunity for optimizations and exposed this issue.

Since we already did many fixes, if it's just one or two more fixes, do you think we could try to keep it?

I don't have a strong opinion, so accepting anyways.

[hoy]

In D155252#4502753, @wenlei wrote:

In D155252#4501305, @hoy wrote:

In D155252#4499632, @wenlei wrote:

In general if the effort to make sure probe has zero discriminator is too much, it makes sense to just rely on discriminator clean up instead during FS discriminator assignment for probes. I'm curious though what exposed this case? IIRC, we didn't hit this assert for a while now.

Yeah, it was good for a while until incrPGO was on. I suspect incrPGO opened more opportunity for optimizations and exposed this issue.

Since we already did many fixes, if it's just one or two more fixes, do you think we could try to keep it?

There doesn't seem to be a very clean fix for this issue. The debug loc merge API (getMergedLocation) takes two DebugLoc params, so to tell if they are from two probes we would need to go above a level but there are quite a few places doing code merge.

llvm/lib/IR/PseudoProbe.cpp

if (const auto *II = dyn_cast<PseudoProbeInst>(&Inst)) {
     Probe.Id = II->getIndex()->getZExtValue();
     Probe.Type = (uint32_t)PseudoProbeType::Block;
     Probe.Attr = II->getAttributes()->getZExtValue();
     Probe.Factor = II->getFactor()->getZExtValue() /
                    (float)PseudoProbeFullDistributionFactor;
     Probe.Discriminator = 0;
     if (const DebugLoc &DLoc = Inst.getDebugLoc())
       Probe.Discriminator = DLoc->getDiscriminator();
-    assert(Probe.Discriminator == 0 &&
-           "Unexpected non-zero FS-discriminator for IR pseudo probes");
     return Probe;
   }
 
   if (isa<CallBase>(&Inst) && !isa<IntrinsicInst>(&Inst))
     return extractProbeFromDiscriminator(Inst);
 
   return std::nullopt;
 }