Page MenuHomePhabricator

[InstCombine] Minor cleanup of cast simplification code [NFC]
ClosedPublic

Authored by mreisinger on Jul 17 2016, 6:17 AM.

Details

Reviewers
vtjnash
grosser
Summary

This patch cleans up parts of InstCombine to raise its compliance with the LLVM coding standards and to increase its readability. The changes and according rationale are summarized in the following:

  • Rename ShouldOptimizeCast() to shouldOptimizeCast() since functions should start with a lower case letter.
  • Move shouldOptimizeCast() from InstCombineCasts.cpp to InstCombineAndOrXor.cpp since it's only used there.
  • Simplify interface of shouldOptimizeCast().
  • Minor code style adaptions in shouldOptimizeCast().
  • Remove the documentation on the function definition of shouldOptimizeCast() since it just repeats the documentation on its declaration. Also enhance the documentation on its declaration with more information describing its intended use and make it doxygen-compliant.
  • Change a comment in foldCastedBitwiseLogic() from fold (logic (cast A), (cast B)) -> (cast (logic A, B)) to fold logic(cast(A), cast(B)) -> cast(logic(A, B)) since the surrounding comments use this format.
  • Remove comment Only do this if the casts both really cause code to be generated. in foldCastedBitwiseLogic() since it just repeats parts of the documentation of shouldOptimizeCast() and does not help to improve readability.
  • Simplify the interface of isEliminableCastPair().
  • Removed the documentation on the function definition of isEliminableCastPair() which only contained obvious statements about its implementation. Instead added more general doxygen-compliant documentation to its declaration.
  • Renamed parameter DoXform of transformZExtIcmp() to DoTransform to make its intention clearer.
  • Moved documentation of transformZExtIcmp() from its definition to its declaration and made it doxygen-compliant.

Diff Detail

Event Timeline

mreisinger updated this revision to Diff 64252.Jul 17 2016, 6:17 AM
mreisinger retitled this revision from to Enable folding of (logic (cast icmp), (cast icmp)) to (cast (logic (icmp), (icmp))).
mreisinger updated this object.
mreisinger added reviewers: grosser, vtjnash.
mreisinger added a subscriber: llvm-commits.
grosser edited edge metadata.Jul 17 2016, 10:35 PM

Hi Matthias,

thank you for the patch. It is technically very solid and despite the comments I will have, is also style-wise already at a good level. I still will provide you with plenty of comments -- most of them further suggestions on style, with the motivation to help you learn more about the style we use in LLVM and also to push your patch from "good" to "exceptional" quality. Future reviews will likely work a lot faster after general concepts have been discussed in the first commits. Feel free to raise concerns if you disagree with some of my suggestions.

One general comment: It seems 95% of your patch consists of documentation and code-style improvements that independently improve code-quality and which are not inter-tangled with the actual change you suggest. I personally prefer to split such changes out of an actual functionality change to upstream them separately. This has the following advantages:

  • The cleanups can be marked as "NFC", no functional change, which makes them a lot easier to review
  • Should the functionality change be incorrect and needs to be reverted, the beneficial cleanups remain in the code base
  • The actual functionality change becomes a lot simpler (in your case 5 lines of code), which allows people skimming over the patch to quickly focus on the central part of this change.

I also suggest - despite the simplicity of the change - to expand your commit message with some more background on why you do something. Here some content that seems interesting for the reader:

InstCombine: Improve documentation of cast-folding logic

  • Why do you rename the function?
  • Why do you move the function between translation units - It seems the function is today only used by the casting logic and consequently belongs conceptually there. Is this right?
  • Mention that you add full doxygen comments
  • Mention that you simplify the interface of isEliminableCastPair

Enable folding ...

The commit message you have today already describes the core change that you apply. However, you spent a lot of time going through commit history to understand _why_ the logic as it exists today was introduced, why the isa<ICmpInst> are around, and why the existing solution seems to be a shortcut that aims to do exactly what you propose, but without making the check if casts can be folded explicit. Instead it bails out for any ICmpInst, even though in many cases this is not necessary and consequently optimization opportunities are missed. Also, you found that some of the functionality you propose today existed earlier, but was dropped in a later commit due to lack of test cases covering this. You could also refer to the original commit messages (SVN ids) in the context of this brief history lesson.

In general commit messages have two purposes. 1) Describe the actual change 2) establish knowledge on how LLVM works and establish a common coding style. Most likely several hundred people will skim over your commit message. Enabling them to learn from your commit (messages) increases the value of your change for LLVM.

It might make sense to make this review about the style changes and introduce a second review that for the actual functionality change.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1222
  • The 'braces' seem unnecessary.
  • The comment does not talk about the innermost condition, no? It seems this is the most important part of this check.
lib/Transforms/InstCombine/InstCombineCasts.cpp
237

Nice cleanup!

265

Nice cleanup.

552
  • DoTransform is clearly more readable
  • What was the motivation of switching operand order here?
lib/Transforms/InstCombine/InstCombineInternal.h
358
  • Drop '\brief'. In general we try to match local style as close as possible, but in respect of LLVM tries to move to

\brief comments without the explcit \brief keyword.

  • "worthy" does not seem to be the right word, I would stay with "worth".
  • Also, please add @param documentations (even though the surrounding code is clearly not very well documented).

Finally, the content of this comment seems slightly confusing. The function you document here contains the code:

  // If this cast is paired with another cast that can be eliminated, we prefer
	​  // to have it eliminated.
	​  if (const CastInst *PrecedingCI = dyn_cast<CastInst>(CastSrc))
	​    if (isEliminableCastPair(PrecedingCI, CI))
	​      return false;

which checks to me if two casts can be "elimintated" -- which to me is the same as "folded together". So in case casts are "folded". the function returns false, which is contrary to what your \brief comment seems to say.

364

Use @returns to describe the return value.

394
  • Use @param to describe DoTransform and @returns to describe the return value.
  • Drop "you just want to", as it does not carry any information
418
  • Describe (briefly) what the function does in the first sentence, rather than that it is implemented as a wrapper to CastInst::isEliminableCastPair.
  • Empty line between single "brief" sentence and the rest.
  • Use @see to refer to other function.
  • Document parameters with @params and return value with @returns.
test/Transforms/InstCombine/fold-casts-of-icmps.ll
5 ↗(On Diff #64252)

I would suggest to add these examples to one of the existing files. Which of the test files is covering the "// fold (logic (cast A), (cast B)) -> (cast (logic A, B))" pattern for the currently handled cases? Is it cast.ll?

Thank you a lot for this extensive feedback, Tobias! I agree that it makes to make this an NFC for the style changes for now. As soon as this is merged I will open a separate PR for the functional changes then. Before updating the patch I also have a few questions (also see my responses to your inline comments).

Why do you move the function between translation units - It seems the function is today only used by the casting logic and consequently belongs conceptually there. Is this right?

shouldFoldCast (or ShouldOptimizeCast when using its original name) only seems to be a helper function for this specific optimization within InstCombineAndOrXor.cpp which is the reason why I preferred to move it there rather than leaving it in InstCombineCast.cpp. However, since its functionality is "cast centric" I could also let it stay there. What would be the more fitting choice in your opinion?

lib/Transforms/InstCombine/InstCombineCasts.cpp
552

What was the motivation of switching operand order here?

IMO it seemed a little bit unpleasant that in the function name we have ZExt before ICmp but the parameter order did not reflect that.

lib/Transforms/InstCombine/InstCombineInternal.h
358

Drop '\brief'. In general we try to match local style as close as possible, but in respect of LLVM tries to move to \brief comments without the explcit \brief keyword.

I was trying to comply with LLVM's "Coding Standards" document here http://llvm.org/docs/CodingStandards.html which proposes the use of the \brief keyword. Maybe this document does not yet reflect the new style goals?

which checks to me if two casts can be "elimintated" -- which to me is the same as "folded together". So in case casts are "folded". the function returns false, which is contrary to what your \brief comment seems to say.

You are right, this seems a little bit confusing. Since shouldFoldCast seems to be only used for this special "(logic (cast icmp), (cast icmp)) to (cast (logic (icmp), (icmp)))" scenario we should maybe make this intention explicit here? For example by also renaming this to something like shouldFoldCastWithinBitwiseLogic or shouldMoveCastAfterBitwiseLogic which would reflect its dedicated use in foldCastedBitwiseLogic more directly? Please also feel free to propose a better naming.

418

This is the original comment, I just moved it from the function definition to its declaration (just added a "\brief"). However, this is no excuse and I'm also in favor of making this comment a little more meaningful. However, since it basically just contains a call to CastInst::isEliminableCastPair (http://llvm.org/docs/doxygen/html/classllvm_1_1CastInst.html#a8b805c54dc1ead67b711a4b1cb72f492) one might be tempted to let it just refer to the documentation there. I will try to provide something useful here.

test/Transforms/InstCombine/fold-casts-of-icmps.ll
5 ↗(On Diff #64252)

That's a good point. Unfortunately, I currently don't know where the original functionality is tested. I will try to figure this out. Since I will separate the tested functionality from this NFC, this will then be part of the next PR.

mreisinger updated this revision to Diff 64323.Jul 18 2016, 8:20 AM
mreisinger retitled this revision from Enable folding of (logic (cast icmp), (cast icmp)) to (cast (logic (icmp), (icmp))) to [InstCombine] NFC: Minor code cleanup.
mreisinger updated this object.
mreisinger edited edge metadata.

I updated the patch now to only contain style changes (see title and summary for detailed information). And you were right Tobias (regarding \brief), since autobrief is enabled \brief can safely be dropped here.

Please feel free to post further comments. I should also mention that I do not yet have commit access, so in case these changes can be merged, somebody with the according rights might have to do this. Thank you!

Best,
Matthias

mreisinger updated this object.Jul 18 2016, 8:22 AM
mreisinger updated this revision to Diff 64331.Jul 18 2016, 8:41 AM

Fixed some typos.

This patch looks great. Just a couple of minor comments left:

  • I would suggest a slightly more descriptive title: [InstCombine] Minor cleanup of cast simplification code [NFC]
  • Two typos (see below)
  • Use present tense in commit message (you are describing the commit, not an event in the past)
lib/Transforms/InstCombine/InstCombineInternal.h
394

interested

395

interested

423

Add reference @see CastInst::isEliminableCastPair

mreisinger retitled this revision from [InstCombine] NFC: Minor code cleanup to [InstCombine] Minor cleanup of cast simplification code [NFC].Jul 18 2016, 9:10 AM
mreisinger updated this object.
mreisinger updated this object.Jul 18 2016, 9:20 AM
mreisinger updated this revision to Diff 64337.Jul 18 2016, 9:24 AM

Thank you for the feedback, I updated the patch accordingly.

Best,
Matthias

majnemer added inline comments.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
1200

Please put returns on their own line.

1204

const auto * would be more concise.

1263–1265

Please clang-format this.

grosser accepted this revision.Jul 18 2016, 9:33 AM
grosser edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 18 2016, 9:33 AM
mreisinger updated this object.
mreisinger edited edge metadata.

Thank you for bringing this to my notice @majnemer.

grosser closed this revision.Jul 19 2016, 2:14 AM

Committed in r275964