This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Restore magic and magicu to a globally visible location
ClosedPublic

Authored by ctetreau on Sep 29 2021, 11:37 AM.

Details

Summary

While these functions are only used in one location in upstream,
it has been reused in multiple downstreams. Restore this file to
a globally visibile location (outside of APInt.h) to eliminate
donwstream breakage and enable potential future reuse.

Additionally, this patch renames types and cleans up
clang-tidy issues.

Diff Detail

Event Timeline

ctetreau created this revision.Sep 29 2021, 11:37 AM
ctetreau requested review of this revision.Sep 29 2021, 11:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 11:37 AM
This revision is now accepted and ready to land.Sep 29 2021, 11:43 AM

I decided to create a new file in llvm/Support rather than take one of the suggestions in Chris's diff thread because:

  • MathExtras.h - I didn't want to add a #include to APInt.h where there wasn't one already. Plus, that file is already pretty big, and most users of it don't need this.
  • somewhere in lib/CodeGen - our usage in my downstream is in lib/Target, which I think demonstrates that this functionality is more broadly useful.

Given this, I think a new file in lib/Support is appropriate.

bogner accepted this revision.Sep 29 2021, 11:47 AM
bogner added a subscriber: bogner.

LGTM with a nitpick

llvm/include/llvm/Support/DivisionMagic.h
12 ↗(On Diff #375973)

Please use the usual include guards (LLVM_SUPPORT_DIVISIONMAGIC_H etc)

ctetreau added inline comments.Sep 29 2021, 12:33 PM
llvm/include/llvm/Support/DivisionMagic.h
12 ↗(On Diff #375973)

OMG, It's like I've never written C++ before! How embarrasing :(

ctetreau updated this revision to Diff 375995.Sep 29 2021, 12:40 PM

add include guard

Cool, thank you for pulling this back, I'm sorry for the breakage. I think that adding this to its own header makes a lot of sense. While you're here, please rename the structs and functions. this shouldn't be called llvm::magic, the names should reference division by a constant somehow. Thanks!

lattner requested changes to this revision.Sep 29 2021, 1:34 PM
This revision now requires changes to proceed.Sep 29 2021, 1:34 PM
ctetreau updated this revision to Diff 376047.Sep 29 2021, 2:59 PM

address code review issues; clean up the interface

craig.topper added inline comments.Sep 29 2021, 3:03 PM
llvm/lib/Support/DivisionByConstantInfo.cpp
25

With the struct being renamed, a variable called mag no longer makes sense. It's not an obvious abbreviation for "magic" if "magic" doesn't appear anywhere.

69

Same here

craig.topper added inline comments.Sep 29 2021, 3:04 PM
llvm/include/llvm/Support/DivisionByConstantInfo.h
13

This doesn't match the file name.

ctetreau updated this revision to Diff 376055.Sep 29 2021, 3:36 PM

address code review issues:

  • fix include guard
  • rename mag to Retval
  • also, make all the variables PascalCase to silence clang-tidy
ctetreau marked 2 inline comments as done.Sep 29 2021, 3:39 PM
ctetreau marked an inline comment as done.
lattner accepted this revision.Sep 29 2021, 4:36 PM

I love it, thank you!

This revision is now accepted and ready to land.Sep 29 2021, 4:36 PM
foad added a subscriber: foad.Sep 30 2021, 2:15 AM
foad added inline comments.
llvm/include/llvm/Support/DivisionByConstantInfo.h
2

Just a nit: do we need "Info" in the name? It seems odd in the filename, and I'm not even sure we need it in the struct names either.

ctetreau edited the summary of this revision. (Show Details)Sep 30 2021, 9:27 AM
ctetreau added inline comments.Oct 1 2021, 9:28 AM
llvm/include/llvm/Support/DivisionByConstantInfo.h
2

It's a bit verbose I agree, but I think it's a (barring abbreviations) minimally descriptive name. It's information about a division by a constant, not a division by a constant.

ctetreau closed this revision.Oct 1 2021, 9:29 AM