[PATCH] D145171: [FSAFDO] Improve FS discriminator encoding
Rong Xu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 6 10:44:31 PST 2023
xur added a comment.
Thanks Hongtao for the reviews and comments. I'll update the patch shortly.
================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:310
ErrorOr<uint64_t>
SampleProfileLoaderBaseImpl<BT>::getInstWeightImpl(const InstructionT &Inst) {
const FunctionSamples *FS = findFunctionSamples(Inst);
----------------
hoy wrote:
> hoy wrote:
> > How about making this function virtual and override it in MIR sample loader? Something like
> >
> >
> >
> > ErrorOr<uint64_t> getInstWeightImpl(const MachineInstr &Inst) {
> > if (ImprovedFSDiscriminator && Inst.isMetaInstruction())
> > return std::error_code();
> > return SampleProfileLoaderBaseImpl<MachineBasicBlock>::getInstWeightImpl(
> > Inst);
> > }
> Or maybe just override the existing virtual function `getInstWeight`, just like `SampleProfileLoader::getInstWeight`
I can do that. This way we don't need shouldIgnoreInst() function.
I have this shouldIgnoreInst() because there are types of instructions in SamplePorfiles try to ignore too. I want to have a unify interface for that. But this seems to be an overkill.
================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:82
+ Ret ^= updateHash(std::to_string(DIL->getLine()));
+ Ret ^= updateHash(DIL->getScope()->getSubprogram()->getLinkageName());
+ }
----------------
hoy wrote:
> `getLinkageName` may return an empty string C functions. We often use a trick to work it around:
>
> // Use linkage name for C++ if possible.
> auto Name = SP->getLinkageName();
> if (Name.empty())
> Name = SP->getName();
>
> Actually there is a similar function `getCallStackHash` in `SampleProfileProbe.cpp`. Do you think it's possible to unify them and place it in a command file like `DebubInfo.h`?
Good to know this. Have see seen cases that empty string leads to hash conflicts?
Will getName() also return empty string?
We can add another interface in DISubprogram? in DebuginfoMetadata.h?
================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:113
+ if (ImprovedFSDiscriminator)
+ LowBitTemp -= 1;
+ unsigned BitMaskBefore = getN1Bits(LowBitTemp);
----------------
hoy wrote:
> Is this fixing a bug of the old encoding?
>
> BTW, should we assert the current pass is always not the base pass or LowBit is never zero?
This is a bug fix. I notice that issue for a while but I did fix it because it changes of some discriminators.
I will add an assert here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145171/new/
https://reviews.llvm.org/D145171
More information about the llvm-commits
mailing list