[PATCH] D19526: Allow unsigned comparisons

[Johannes Doerfert via llvm-commits]

jdoerfert added inline comments.

================
Comment at: test/ScopInfo/simple_loop_unsigned.ll:13
@@ +12,3 @@
+; CHECK:      Arrays {
+; CHECK-NEXT:     i64 MemRef_a[*]; // Element size 8
+; CHECK-NEXT: }
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Meinersbur wrote:
> > > Why is this significant for the test case?
> > Idk. Most of the check lines are not. Is this a problem now?
> Not a problem, but makes it harder to understand what's the point of the test.
> 
> I could understand why checking the Statement because of its domain, but I don't know why checking the accessed array and its size should be relevant.
Personally, I think this is overkill to argue about 3 trivial check lines of a test case that has 12 lines of IR. Anyway, you seem to care a lot about it and I will remove them.

================
Comment at: test/ScopInfo/simple_loop_unsigned_2.ll:36
@@ +35,3 @@
+  store i64 %i, i64* %scevgep
+  %i.dec = add nsw i64 %i, -1
+  %exitcond = icmp ugt i64 %i.dec, 0
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Meinersbur wrote:
> > > Since `i` is unsigned, why does it have a nsw flag?
> > that wasn't me (I think). Is it really important if it has this flag or not?
> This is a brand new test case, so who else could have added it?
> 
> The problem I see is that the commented C-code does not accurately represent the IR, hence does a disfavor when trying to understand the code. If you do not intend to keep the C-representation in-sync with the IR, you can just remove it.
> This is a brand new test case, so who else could have added it?
Clang? One of our preparation passes? Somebody else and I just copied an existing test case as a basis for this one?

> The problem I see is that the commented C-code does not accurately represent the IR, hence does a disfavor when trying to understand the code. If you do not intend to keep the C-representation in-sync with the IR, you can just remove it.
Did you actually compile the code to IR? The nsw you seem to care about is always there. Regarding the mismatch of C and IR, your comment seems rather odd. We surely can just drop all the C code but I think it helps people to understand the structure of the test case, even though it is/cannot always represent the IR exactly. In general I think this is again a discussion that is out of place and if you care about it I would suggest you move it to the mailinglist.


http://reviews.llvm.org/D19526