[PATCH] D88609: Use uint64_t for branch weights instead of uint32_t

[dmajor via Phabricator via cfe-commits]

dmajor added a comment.

After 10f2a0d662d8 
 our 
self-host PGO builds are hitting:
llvm::BranchProbability::getBranchProbability(uint64_t, uint64_t): Assertion 
`Numerator <= Denominator && "Probability cannot be bigger than 1!"' failed.

https://treeherder.mozilla.org/logviewer?job_id=320324631&repo=try&lineNumber=52597
 has the log as well as the .cpp/.sh reproducers, but I assume you'd also need 
the merged.profdata file, and our bots haven't been taught to save those. I can 
try to add that ability next week if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88609: Use uint64_t for branch weights instead of uint32_t

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

The stack trace helped me come up with my own repro, reverting.
It's more uint64_t overflow issues. This might be the reason branch_weights 
were initially i32 with casts in a bunch of places around LLVM. Not sure if I 
should try and fix all usages or just keep branch_weights as i32.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88609: Use uint64_t for branch weights instead of uint32_t

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

This was reverted due to crashes caused by adding branch weights overflowing 
uint64_t. Now in BranchProbabilityInfo.cpp, if the sum of weights overflows, 
scale down all weights by UINT32_MAX (See `ScaleWeights()`). Is that ok?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88609: Use uint64_t for branch weights instead of uint32_t

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a subscriber: thakis.
kiranchandramohan added a comment.

@thakis Can you revert the testcase fix in MLIR also which was necessary after 
this patch? https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88609: Use uint64_t for branch weights instead of uint32_t

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Updated mlir test also to include this change.
https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits