This is an archive of the discontinued LLVM Phabricator instance.

[NFC] format InstructionSimplify & lowerCaseFunctionNames
ClosedPublic

Authored by simoll on Jun 2 2022, 8:43 AM.

Details

Summary

Clang-format InstructionSimplify and convert all "FunctionName"s to "functionName".
This patch does touch a lot of files but gets done with the cleanup of InstructionSimplify in one commit.

This is the alternative to the less invasive clang-format only patch: D126783

Diff Detail

Event Timeline

simoll created this revision.Jun 2 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 8:43 AM
simoll requested review of this revision.Jun 2 2022, 8:43 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
simoll updated this revision to Diff 433768.Jun 2 2022, 9:02 AM

Also use the new function names in the unit tests..

As with the alternative, this needs more agreements, but in this case, it touches more files and the probability it will create merge conflicts with other people's in-flight work increases.

reames added a comment.Jun 2 2022, 9:07 AM

I see little value in the function renames. Not actively opposed, but personally, I would not bother.

spatel added a comment.Jun 2 2022, 9:37 AM

I should have been clearer with my original suggestion - I was only thinking of changing the static/internal function names within InstSimplify itself, so it would still be a one-file patch.

I realize that doesn't fix the overall formatting mess of LLVM, but it would hopefully make InstructionSimplify.cpp alone a little easier to read.

I don't object to the larger change, but I only work on the main branch. AFAIK, we've avoided doing something this big because it makes life harder for maintainers of private forks.

foad added a comment.Jun 6 2022, 4:09 AM

No objection from me, and I do maintain downstream forks. Can you provide a simple one-line command that I can run to make the equivalent changes in my downstream?

spatel added a comment.Jun 6 2022, 5:05 AM

No objection from me, and I do maintain downstream forks. Can you provide a simple one-line command that I can run to make the equivalent changes in my downstream?

I'm guessing this used a basic grep for "Simplify", but that needs refinement if we proceed - see inline comments for some mistakes.

llvm/lib/Analysis/InstructionSimplify.cpp
394

This should be updated.

4155

We don't want this change.

4596

We don't want this change.

5570

This should be updated.

simoll updated this revision to Diff 434754.Jun 7 2022, 3:17 AM
simoll marked 4 inline comments as done.

Cleanup, repair function names, comments, variables

simoll added a comment.Jun 7 2022, 3:22 AM

No objection from me, and I do maintain downstream forks. Can you provide a simple one-line command that I can run to make the equivalent changes in my downstream?

Apart from applying the patch, not really. This is clang-format + lower-case all function names in InstCombine.cpp/h + compile, wait for an error and lower-case the highlighted function name in the error message, repeat.

No objection from me, and I do maintain downstream forks. Can you provide a simple one-line command that I can run to make the equivalent changes in my downstream?

I'm guessing this used a basic grep for "Simplify", but that needs refinement if we proceed - see inline comments for some mistakes.

Yes. I've started with string replacement in InstSimplify. The latest Diff is with manual refinements, that is going through the Diff line by line and mending unintentional changes.

spatel accepted this revision.Jun 8 2022, 12:58 PM

LGTM

llvm/lib/Analysis/InstructionSimplify.cpp
2553

Missed a set of capitalized static functions starting here.

This revision is now accepted and ready to land.Jun 8 2022, 12:58 PM
simoll updated this revision to Diff 435526.Jun 9 2022, 6:24 AM
  • lower case more static functions.
  • rebased.
simoll marked an inline comment as done.Jun 9 2022, 6:29 AM

We got one accept and no opposition. This passes all testing locally. If pre-merge checks are ok (except for unrelated breakage), I will go forward and commit this.

rengolin accepted this revision.Jun 9 2022, 6:33 AM

We got one accept and no opposition. This passes all testing locally. If pre-merge checks are ok (except for unrelated breakage), I will go forward and commit this.

LGTM! :)

This revision was landed with ongoing or failed builds.Jun 9 2022, 7:10 AM
This revision was automatically updated to reflect the committed changes.