[PATCH] D22449: Enable folding of (logic (cast icmp), (cast icmp)) to (cast (logic (icmp), (icmp)))

[Tobias Grosser via llvm-commits]

grosser added a comment.

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.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1222
@@ +1221,3 @@
+    }
+  }
+
----------------
- 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.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:237
@@ -244,1 +236,3 @@
+  Instruction::CastOps firstOp = Instruction::CastOps(CI1->getOpcode());
+  Instruction::CastOps secondOp = Instruction::CastOps(CI2->getOpcode());
   Type *SrcIntPtrTy =
----------------
Nice cleanup!

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:265
@@ -294,3 +264,3 @@
     if (Instruction::CastOps opc =
-            isEliminableCastPair(CSrc, CI.getOpcode(), CI.getType(), DL)) {
+            isEliminableCastPair(CSrc, &CI)) {
       // The first cast (CSrc) is eliminable so we need to fix up or replace
----------------
Nice cleanup.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:552
@@ -585,1 +551,3 @@
+Instruction *InstCombiner::transformZExtICmp(ZExtInst &CI, ICmpInst *ICI,
+                                             bool DoTransform) {
   // If we are just checking for a icmp eq of a single bit and zext'ing it
----------------
- DoTransform is clearly more readable

- What was the motivation of switching operand order here?



================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:358
@@ -357,2 +357,3 @@
 
-  /// \brief Classify whether a cast is worth optimizing.
+  /// \brief Classify whether it is worthy to remove a cast by folding it with
+  /// another cast.
----------------
- 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.



================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:365
@@ +364,3 @@
+  /// Returns true if this cast actually results in any code being generated and
+  /// if it cannot already be eliminated by some other transformation.
+  bool shouldFoldCast(CastInst *CI);
----------------
Use @returns to describe the return value.

================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:393
@@ +392,3 @@
+  ///
+  /// Pass `false` for \p DoTransform if you just want to test whether the
+  /// given (zext icmp) would be transformed. Pass `true` if you also want to
----------------
- Use @param to describe DoTransform and @returns to describe the return value.

- Drop "you just want to", as it does not carry any information

================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:411
@@ +410,3 @@
+  /// It simply extracts arguments and returns what that function returns.
+  Instruction::CastOps isEliminableCastPair(const CastInst *CI1,
+                                            const CastInst *CI2);
----------------
- 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.

================
Comment at: test/Transforms/InstCombine/fold-casts-of-icmps.ll:5
@@ +4,3 @@
+; (logic (cast A), (cast B)) to (cast (logic A, B)) also works if A or B are
+; icmp instructions.
+
----------------
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?


https://reviews.llvm.org/D22449