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

[Tobias Grosser via llvm-commits]

On Mon, Jul 18, 2016, at 11:26 AM, Matthias J. Reisinger wrote:
> 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).

Cool.
 
> > 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?

I let you decide what is more suited. Both could make sense. Just make
sure you explain your reasoning in the commit message.
 
> ================
> 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.

Please check in the surrounding code what is the common pattern. If the
new writing is clearly more consistent, please provide the explanation
in the commit message. If it is just a minor change, I would leave the
code as it is. People often have different ideas about style and
switching back and forther between personal preferences only causes
noise. 

> ================
> 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?

Please check this thread. If the conclusion is to not use \brief, you
might ping them to update the style documents:

 http://lists.llvm.org/pipermail/llvm-dev/2015-May/085973.html

> > 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.

I prefer "optimize" over "fold". shouldOptimizeCast actually seems to
make a lot of sense. We could try to explain that this
is bitwise specific, but I don't see it giving a big benefit. Hence, I
would just leave the name as it is and save the time on discussing this
topic.
 
> ================
> 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.

Just give a brief one-sentence explanation what the function does and
then reference to the other documentation with @see.
 
> ================
> 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.

Yes, please check this out.

Tobias