[PATCH] D48828: [InstSimplify] fold extracting from std::pair (1/2)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Thanks for splitting it up. This is close to good IMO - just a few minor points:

1. Please add/adjust the tests with baseline checks as a preliminary step; we don't want to lose those in case the code change gets reverted.
2. Seeing this diff on its own makes it clear that we're overspecifying the more general SimplifyDemandedBits transform (what about the case where the shifts are in the opposite order?). That should be noted as a code comment and in the commit message.*
3. I don't expect any compile-time problems given that the computeKnownBits is buried under the other pattern checks, but be aware of that concern and watch for regressions.

- It has been discussed before that SimplifyDemandedBits really shouldn't be included in InstCombine; it should be its own pass. If that structural change was made, would it make adjusting the optimization pipeline a more appealing solution than this?



================
Comment at: test/Transforms/InstSimplify/shift.ll:218
+  %tmp3 = shl nuw <2 x i64> %tmp1, <i64 32, i64 32>
+  %tmp4 = or <2 x i64> %tmp2, %tmp3
+  %tmp5 = lshr <2 x i64> %tmp4, <i64 32, i64 32>
----------------
Swap the 'or' operands here so we have coverage for the commuted case?
In general, I like to see a test comment that points that out too, so we know what's changing between the tests.
Also, it's a matter of taste, but the more common test format would put the test comment above the test definition, so it's clearly separated from the auto-generated CHECK lines.


https://reviews.llvm.org/D48828