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

[Matthias J. Reisinger via llvm-commits]

mreisinger added a comment.

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?


================
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
----------------
grosser wrote:
> - DoTransform is clearly more readable
> 
> - What was the motivation of switching operand order here?
> 
> 
> 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.

================
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.
----------------
grosser wrote:
> - 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.
> 
> 
> 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.

================
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);
----------------
grosser wrote:
> - 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.
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.

================
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.
+
----------------
grosser wrote:
> 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?
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.


https://reviews.llvm.org/D22449