This is motivated by PR-30483. New testcases and comments in the code explain the opportunities. Still more work can be done for this. I will open new PR for remaining cases (including those that Sanjay mentioned in the PR 30483)
Diff Detail
Event Timeline
I know we want to expand the patterns that are matched beyond what's included in this patch, but it's not clear to me why we need IsAnyBitSet() for these cases. Eg, there are no tests for the 'icmp ugt' case.
Is it simpler to just use more direct pattern matching for now and then see how it can be recoded as we add cases?
I'm also wondering if there's some way to reuse the existing decomposeBitTestICmp() or the code in foldSelectInstWithICmp() that is marked with a FIXME. We also have isSignBitCheck() and isSignTest() in InstCombineCompares.cpp. There must be some way to refactor all of this logic because we're looking for similar patterns in several different places?
test/Transforms/InstCombine/and-or-icmps.ll | ||
---|---|---|
54–60 | You can use utils/update_test_checks.py to auto-generate better checks for all of these tests. That's how the existing checks in this file were produced. |
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
744–750 | A tuple with multiple bool types can be a little confusing. I'd prefer a little struct. At the very least, this needs to be documented. | |
751 | Please follow the LLVM naming convention. | |
752–754 | Prefer early return to reduce indentation. | |
752–755 | Prefer auto * | |
756–769 | Prefer std::initialzier_list syntax here to reduce visual noise. | |
762–765 | These parenthesis are not necessary. | |
834–835 | auto * | |
836 | Please stick the return on its own line. |
IsAnyBitSet factors out common pattern matching logic in FoldAndOFICmp and FoldXorOfICmp. I can add testcase for UGT case. DecomposeBitTestICmp will be helpful to be called for extending the current functionality. Currently IsAnyBitSet expects patterns similar to those generated by decomposeBitTestICmp so I believe we can first call decomposeBitTestICmp and then IsAnyBitSet to extend the functionality. I prefer to leave that as an extension of the current functionality. isSignBitCheck is a similar but different functionality because it looks at a single special bit. IsAnyBitSet looks at masks that might have more than one bit. I also looked at the FIXME in simplifySelectWithICmpCond if that is what you mean. It says the logic is similar to decomposeBitTestICmp. So given what I said about decomposeBitTestICmp this is a separate issue.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
744–750 | I figured that I will need to add some comments even if I have a struct, because the field names will not be clear enough. So I concluded creating a struct will not be a significant help to clarify things and so I added explanation to the comments before the function to explain each component of the return value. Will update the patch shortly. |
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
744–750 | Actually, I changed my mind about this. Will add an struct and update the patch. |
Thanks David for the review. I have tested this with test-suite. I forgot to do so, after I made first set of changes.
@spatel @eli.friedman Could one of you please look at this patch? @majnemer said he is busy and will be even busier next week.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
747–748 | Is the Valid field necessary? The callers can always just check if (!Mask) ? | |
755–758 | I've scanned over this a few times, and it just never reads clearly to me. Can you add comments and example patterns, so we'll know how this will be used? | |
762–763 | This was not updated to address the earlier review comment. | |
765 | Please use m_APInt here or add a TODO comment because this won't work for vectors. | |
776–778 | The UGT case still has no test coverage? | |
834–835 | Same as above - use m_APInt or add a TODO about the lack of vector support. | |
840 | Can we check that RHSCst == LHSCst and remove the getBitWidth checks below? | |
844–846 | This needs a comment that shows the pattern we are matching and what it folds to. | |
940 | Can we check that RHSCst == LHSCst and remove the getBitWidth checks below? | |
946–952 | This needs an explanatory comment with an example. | |
test/Transforms/InstCombine/and-or-icmps.ll | ||
54–56 | Each test should have a comment to explain the motivation. Ie, it's good to have negative tests, but I can't tell from glancing at these why we would do the transform in some cases and skip it in the others. |
Will shortly update the patch.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
755–758 | added a comment. I hope that make is it more readable. | |
840 | See the respond to a similar comment below. | |
940 | getBitWidth is used for the mask which is operand of the and operator. RHSCst and LHSCst are operands of icmp instructions. I double checked the set of conditions that I have used below and they are all required. | |
946–952 | Example is provided in lines 945-946. (line numbers may change after updating the review). Will revise the comment to be more general and contains more details. |
Thanks for adding the code and test comments - this is much easier to follow now. See inline comments for a couple of small items.
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
758–766 | Simplify? ConstantInt *Mask; if (!match(LHS, m_And(m_Value(), m_ConstantInt(Mask)))) return BGC; | |
840 | Sorry for not seeing it, but I don't know how it's possible that: (*BGC1.Mask == *BGC2.Mask) && (BGC1.Mask->getBitWidth() != BGC2.Mask->getBitWidth()) Can you add a test case to show where that happens? To be clear, my proposal is to change the initial check: if (RHSCst->isZero() && LHSCst->isZero()) to: if (RHSCst->isZero() && LHSCst == RHSCst) And then remove the bit width check from the later 'if' statement. The operand types of icmp and xor/and must match, so I thought that would be sufficient? | |
2799 | Period at end of sentence. |
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
758–766 | Sure. | |
840 | operator== that is called with (*BGC1.Mask == *BGC2.Mask) has an assertion that the two masks has the same bitwidth. IIUC, if we want to do LHSCst == RHSCst the same operator== is called, and so we would need to check the bit widths of RHSCst and LHSCst. So overall it does not make a difference. | |
2799 | Sure. |
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
840 | *BGC1.Mask == *BGC2.Mask is the APInt operator== and yes, it asserts on bitwidth. |
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | ||
---|---|---|
840 | Sorry, I misinterpreted the code. Will change. |
Sorry for not noticing this earlier, but InstCombine always canonicalizes the 'icmp ugt X, 0' cases to 'icmp ne X, 0' (see foldICmpUsingKnownBits()).
Can you just assert that the predicate is not 'ugt' in isAnyBitSet() and simplify that a bit more? Ie, we don't need a switch if we know that eq/ne are the only predicate possibilities for this transform.
"assert" is probably too strong...I've made that mistake before. :)
But I think it's still safe to ignore the 'ugt' case and assume that InstCombine will churn that away sooner or later.
You are right (I just tried it :-) If that is the only concern is it possible to approve this and I will remove that line from the switch stmt. (I think it does not hurt to leave the testcases though) and commit.
Yes, it's good to keep the tests to make sure the underlying assumption doesn't break.
LGTM - although I don't think we need a switch for just 2 cases. :)
Is the Valid field necessary? The callers can always just check if (!Mask) ?