In D151897#4400309, @aportnoy wrote:

@manas Thank you for looking at this! After I rebase onto main using git, what are my steps to update this revision? How do I restart the buildbot? Or does it run automatically after I update the revision? Sorry, first time using the LLVM workflow.

Can you rebase it to latest and restart buildbot? I see no reason it should fail with an OOM.

In D151270#4385283, @mehdi_amini wrote:

The bufferization crash wouldn't be solved by this change anyway would it? Would bufferization handles %r = "arith.select"(%cond, %t, %f) : (tensor<42xi1>, tensor<42xi32>, tensor<42xi32>) -> tensor<42xi32> ?
`

That's correct. I will patch a fix for the crash after this. Yes the bufferization will crash for above example too.

Remove Non0RankPred

In D151270#4379861, @mehdi_amini wrote:

In D151270#4379805, @manas wrote:

In D151270#4379785, @mehdi_amini wrote:

This test looks correct to me, if it fails then you have something incorrect with you check.

I believe we want to prevent any `tensor<i1> in the arith.select's condition.

Why would we? As mentioned before, the "no broadcast" rule is enough: that is "either scalar or shape matching" describe what this op should support, I don't quite get why rank-0 should be special? (the problem is indeed that rank-0 is special right now and I believe the point of this fix should be to make it not special)

In D151270#4379785, @mehdi_amini wrote:

This test looks correct to me, if it fails then you have something incorrect with you check.

In D151270#4379692, @mehdi_amini wrote:

LGTM
(do you have commit access?)

I think I do. I will push it. Thanks a lot for reviewing this!

Do dead-code elimination

Add op type constraint over SelectOp operands

Remove irrelevant docs

Rename type to ScalarConditionOrMatchingShape

In D151270#4366445, @mehdi_amini wrote:

I am not sure that the appropriate description is « book-like of non zero rank ».
To me it is rather: « scalar condition or matching shape » here.

This test contains tensor<i1> and I am not sure if I should edit the CHECKs or remove the test altogether (as it will not check anything about linalg.generic, the reason why it was placed there).

Fix typo

Hi, I came across this due to changes in Static Analyzer. One of the changes is not related to the Github issue mentioned above. Maybe we can drop that file?

Gentle ping.

In D140086#4010175, @steakhal wrote:

This test fails.

void testfoo(unsigned char u, signed int s) {
  if (u >= 253 && u <= 255 && s < INT_MAX - 2) {
    // u: [253, 254], s: [INT_MIN, INT_MAX - 2]
    clang_analyzer_eval(u != s); // expected-warning{{UNKNOWN}}
                                 // but returns TRUE
  }
}

I feel like we have something to talk about.
When I do the review pro bono, I'd like to focus on higher-level issues and let the submitter deal with the smaller concerns.

Re-introduce bitwidth comparison

Remove redundant branches

In D140086#3998426, @steakhal wrote:

Thanks for going the extra mile to address this last thing. I really appreciate it.
I've got only a few minor comments and suggestions.

I'd recommend spell-checking the comments and the summary of this revision.
See my technical comments inline.

Rebase

Rebase

Assert over non-empty ranges and add fixme for not handled cases

In D112621#3978285, @steakhal wrote:

For the test cases where we should be able to prove the case but return Unknown instead, should be marked by a FIXME stating the expected value.
Approximating a concrete value with Unknown is (almost) always correct. So, in this case, you don't need to go and fix them unless you want to do the extra mile.

Add more tests for complete coverage

Fix test cases

In D112621#3949400, @steakhal wrote:

Analysis/constant-folding.c seems to fail.
Please run the check-clang-analysis build target to see what fails and investigate it.

Ping

Thanks @martong and @ASDenysPetrov for the feedback. I utilized (u != n) to disallow bifurcation in test cases.

Fix casting to bigger/unsigned type and add tests with multiple ranges in RangeSet

Rebase and fix comments

Considering @ASDenysPetrov 's example of LHS = [1, 2] U [8, 9] and RHS = [5, 6], I constructed a test case as following:

Remove filling gaps and convert, use castTo, and add tests for short-ushort,
char-uchar pairs

The coverage showing unreachability of VisitBinaryOperator<BO_NE> for concrete integer cases.

Add concrete tests for same and different types

I was busy with other stuff. I will take a look at it now.

Format constant-folding.c

Fix comments and rebase

@steakhal apologies for holding onto this for so long. I managed to fix previously untestable test cases. The issue was that I was building expressions as (u1 != u2) != 0 and the solver was canonicalizing this to an equivalent BO_EQ expression. That's why, it wasn't reaching VisitBinaryOperator<BO_NE>(). So I changed all tests as following: (u1 != u2) (removing the latter != 0 part. Also, I utilized the getConcreteValue() for checking of single APSInt in the RangeSets.

Fix test cases to make them reachable via VisiBinaryOperator<BO_NE> and using getConcreteValue()

Buildbots are happy. I think we can land this. @steakhal do you mind landing it?

I rebased to see if it satisfies the buildbot.

Rebase

This patch fixes #53967.

Closing this as this is mistakenly a duplicate of existing differential D112621.

I have made few changes:

Fix comparison between different types and add a test to check it

In D112621#3090450, @steakhal wrote:

It seems like the test does not exercise the modified code segment.
Please investigate, and make sure that the code you submit is actually exercised by the test you provide.

@steakhal @martong This patch solves the comparison between different types by bypassing the RangeSets first to VisitBinaryOperator which coarses/converts, and then only it checks for any True/False ranges via comparison. This is similar to what happens with BO_Rem, BO_And etc. as well.

Gentle ping! I think landing it will make collecting analyzer information easier (in terms of faster build) as more (appropriate) cmake options can be passed via cmdline.

Based on the information from debugger, the logs show RangeSets [0, 255] and [INT_MIN, 0] from different types are causing the failure.
I tried producing a compact test case. The essential part is the comparison between different types, as in this case unsigned int and int. In other binary operators (BO_And, BO_Rem, etc.), this gets handled by coarsing RangeSets to Ranges and converting those Ranges to resulting type for comparison. This is missing in BO_NE. I suppose, this may be the reason behind the signedness mismatch.

I have received a failed build on clang-ppc64be-linux (https://lab.llvm.org/buildbot/#/builders/52/builds/11704). Although, I haven't verified the commit. Is there something wrong with this commit? Is the function VisitBinaryOperator<BO_NE> reachable?

I'll take a look at this over the weekend.

In D107312#3036421, @steakhal wrote:

I'm not using this script. I'm assuming you run it and verified that it works.
Thanks for cleaning this up.

In D106102#3036474, @steakhal wrote:

In D106102#3036220, @manas wrote:

I do not have landing rights.

Please add your name and email on whom behalf I should commit this patch. Mine is Balazs Benics<balazs.benics@sigmatechnology.se>

Gentle ping.

In D106102#3035598, @steakhal wrote:

Good work. Land it.

Fix typo

In D106102#3032888, @steakhal wrote:

Okay, I don't see any problems besides this typo.

Oops!

Apply mentioned edits in testcases

In D106102#3029633, @martong wrote:

In D106102#3028584, @manas wrote:

The pre-merge checks fail due to the patch being unable to get applied. The troubleshooting suggest to update the patch via arc diff `git merge-base HEAD origin` --update D106102 and it would include all local changes into that patch. Shouldn't I avoid including all local changes, as some local changes are still WIP?

I guess HEAD should refer to the commit that represents this patch, in this case that is ce82443c69be. And I believe you local changes that are not committed should not affect arc diff at all. But, as a precaution, you could first stash your local changes before calling arc. Hope this helps.

The pre-merge checks fail due to the patch being unable to get applied. The troubleshooting suggest to update the patch via arc diff `git merge-base HEAD origin` --update D106102 and it would include all local changes into that patch. Shouldn't I avoid including all local changes, as some local changes are still WIP?

Rebase

Move method to a specialized template for VisitBinaryOperator

@vsavchenko should we land this?

Upgrade cmake to 3.21.3

Rebase

I haven't tried specializing that VisitBinaryOperator method which converts Ranges from RangeSets (as @vsavchenko mentioned). Should this case for NE stay here in the switch or move?

In D105273#2891921, @manas wrote:

Here is the proof using Z3: https://gist.github.com/weirdsmiley/8a35a0e1f55f310e3566cbd47555491a

I have updated the proof for Add: https://gist.github.com/weirdsmiley/ad6a9dbf3370e96d29f9e90068931d25