[PATCH] D48584: [InstCombine] Delay foldICmpUsingKnownBits until simple transforms are done

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D48584#1146822, @spatel wrote:

> In https://reviews.llvm.org/D48584#1144405, @mkazantsev wrote:
>
> > I think that these changes are practically either unimportant or positive. SCEV has reasons to prefer `lt/gt` comparisons over `ne/eq` because in certain cases it can derive more facts from it, however I'm not sure whether it has real profit anywhere or not. I can test it on our Java benchmarks, but don't have environment to run C++ clang tests properly, so I'd appreciate if you could help with it.
>
>
> Ok - I need to get a test-suite machine set up again. I'll try to have some data by tomorrow.


I don't see anything above the noise with this patch applied, so we should be ok here.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4707-4709
+  // If nothing helped, let's try to reason basing on known bits. These
+  // transforms can produce bit arithmetics that is hard to analyze by other
+  // passes, so we use it as the last resort if we cannot do anything better.
----------------
I'd state this more like:
"This may be expensive in compile-time, and transforms based on known bits can make further analysis more difficult, so we use it as the last resort..."


================
Comment at: test/Transforms/InstCombine/icmp_sdiv_with_and_without_range.ll:4-5
+
+target datalayout = "e-i64:64-f80:128-n8:16:32:64"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
lebedev.ri wrote:
> Could you add comment what this tests, and commit as-is [and rebase this]?
> Also, i'm not sure these two lines are needed.
Please commit the test file to trunk with the baseline CHECK lines, and then rebase this patch, so we have a record of the bug (also add a comment referencing/linking PR37636).


https://reviews.llvm.org/D48584