ping.

thanks for the review @jdoerfert . Will incorporate these changes and rebase on landed D76140.

In D76140#1957416, @reames wrote:

LGTM again, with minor change.

will update it.

p.s. Sorry for missing the functional issue the first time. All of the test changes should have made the issue obvious, but despite reading the LangRef description of signext, I somehow managed to miss the separation between ABI and optimization attributes.

thanks for the review Philip and pointing out the problem. All of us had missed the functional issue the first time around.

fixed missing code left out during rebase.

fixed buildbot failure. see above comments and added testcase test8.

whitelist valid return attributes and only add those. Added testcase for signext.

In D44429#1953211, @anna wrote:

Hi,

It looks like there's some UB in this code. Pls see this failure: https://reviews.llvm.org/D76140#1953201
I'm not sure how to go about fixing the MsgPackReader code.
@scott.linder Could you PTAL?

see above comment.

In D76140#1953201, @anna wrote:

I got a failure in one of the binaryFormats:
lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/MsgPackReader.cpp

Attributes 'zeroext and signext' are incompatible!
  %rev.i.i.i.i.i.i.i.i = tail call signext zeroext i16 @llvm.bswap.i16(i16 %ret.0.copyload.i.i.i.i) #6
in function _ZN4llvm7msgpack6Reader7readIntIsEENS_8ExpectedIbEERNS0_6ObjectE
fatal error: error in backend: Broken function found, compilation aborted!

I believe this just showcases undefined behaviour since we were having a returnValue (i.e. call) with an incomptable attribute compared to the return attribute on the callsite.

It looks like there's some UB in this code. Pls see this failure: https://reviews.llvm.org/D76140#1953201
I'm not sure how to go about fixing the MsgPackReader code.
@scott.linder Could you PTAL?

I got a failure in one of the binaryFormats:
lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/MsgPackReader.cpp

Attributes 'zeroext and signext' are incompatible!
  %rev.i.i.i.i.i.i.i.i = tail call signext zeroext i16 @llvm.bswap.i16(i16 %ret.0.copyload.i.i.i.i) #6
in function _ZN4llvm7msgpack6Reader7readIntIsEENS_8ExpectedIbEERNS0_6ObjectE
fatal error: error in backend: Broken function found, compilation aborted!

addressed review comments. Added two test cases: deref value being different, inlined callee body better optimized compared to callee.

addressed review comments

NFC w.r.t prev diff. Use VMap.lookup instead of a lambda which does the same.

Just chiming in here. We are planning to use addrspacecast for pointer width conversion (32 to 64 bit and vice-versa). Does not change the memory in anyway, i.e. if we lowered the addrspacecast into an IR call, that call could be marked argmemonly readnone. However, it is *not* a simple sign-extend/truncate either.
I've added some comments inline. Also, I think we have the scope to improve optimizations for addrspacecast at IR level, which is currently limited because different targets have varying lowering for addrspacecast.

ping.

Noticed while adding couple more tests, there were 2 bugs:

use isGuaranteedToTransferExecutionToSuccessor instead of MayThrow

refactored code, made asserts cleaner.

fixed clang tests. rot-intrinsics.c testcase has 5 different RUNs with 3 prefixes. Depending on target-triple, the attribute is added to the caller, so I've disabled the optimization for that specific test with -update-return-attrs=false

Will avoid confusing usage with cleaner asserts.

addressed review comments. fixed CI failure related to clang-tidy.

In D76140#1930078, @reames wrote:

In D76140#1927909, @anna wrote:

the test failures are related to attributes now being added to the calls in clang tests. All of these tests are using llvm intrinsics. Should we just disable this for intrinsic calls for now? Not sure how to update clang tests in the same patch as llvm project. Should be doable though.

Er, git is monorepo. Those tests should be checked out alongside your LLVM copy. You just need to build clang. You can and should simply update the tests in the same patch.

ah yes, I'd build llvm within it's own subdir without enabling any other projects. Build worked. thanks.

the test failures are related to attributes now being added to the calls in clang tests. All of these tests are using llvm intrinsics. Should we just disable this for intrinsic calls for now? Not sure how to update clang tests in the same patch as llvm project. Should be doable though.

addressed review comments. The analysis is conservative and test cases added show the negative cases where we should not backward propagate the attribute to the call (i.e. return value operand).

fixed clang format. NFC w.r.t. prev diff.

will update patch and place for review.

talked with Philip offline. We just need the check for non-trapping instructions between the return value (i.e. the call) and the return instruction. In other cases, it is either correct to propagate the nonnull-ness or it showcased a UB that already existed in the program.

In D76140#1922555, @reames wrote:

Anna, I'd encourage you to go very narrow here. We can resolve the correlated throw case with the following: require operand of return to be call instruction which is less than small constant window of non-trapping instructions before the return. (i.e. start with previous node) If we allow bitcasts, that provides reasonable coverage. We can always fallback to assumes as noted.

We don't need to be hugely general analysis wise to be very useful. Calls in tail positions or loads in analogous are very common. We should handle that obvious case.

hmm. yes, I can see this getting complicated for design and correctness. Thanks for the example @jdoerfert.

In D76140#1922292, @reames wrote:

In D76140#1922162, @jdoerfert wrote:

In addition to @reames correctness concerns this is not valid for a different reason. You cannot backwards propagate information in the general case:

__attribute__((nonnull)) int *foo(int c) {
   int *A = unknown();
   int *B = unknown();
   do_sth_with_ptr_and_may_use_nonnull(A, B);
   return c ? A : B; // or if (C) return A; else return B;
}

Knowing foo returns a nonnull value cannot be used to annotate A or B.

Good catch, I'd missed that.

Just noticed this. I think we can special case this to: the return and def call are in the same basic block (it will take care of the if-else in the example) and has only one use which is the return. The latter takes care of avoiding incorrect semantics in that call do_sth_with_ptr_and_may_use_nonnull or any other use that depends on knowing which of the ptr is non-null.

addressed review comments. This handles all attributes on the call and only done after the cloning of the inlined body.
Tests added for attributes and making sure the original callee does not have the attributes added within its body.

removed extra line. cleaned up whiitespace. NFC w.r.t. prev diff.

changed to early-return, updated comments and test.

In D75704#1909467, @dantrushin wrote:

LGTM, but what about test failure? Please fix it

fixed dominator error.

abandon revision - too old and context lost.

abandoning this change - lost context.

hi, thanks guys. Sorry for the abnormally late response (was on mat leave). Yes, looks like this can be abandoned.

ping

lgtm.

The scalar indices don't appear to be a problem on a scalar gep, we even had a test for that.

just to note it is not a scalar gep - this is a vector gep which has a scalar index.

LGTM w/ comments.

need to update comments and address review comments. moving out of queue for now.

LGTM!

thanks for reviewing the change Brian! Will land it soon.

This has passed all internal fuzzer runs - the DT verification failure tests have been added as test cases here after reduction.

we need to update domInfo for exits and those reachable from the exits.

In D56284#1348783, @kuhar wrote:

In D56284#1348747, @anna wrote:

I've got a naive question about the DTU: Does the automatic updater handle the ImmediateDominator "automatically" for the remaining nodes in the dom tree once identify that one of the nodes in the DT has a change in IDom?

Exactly. Given a set of CFG updates, if figures out how to change the DomTree and PostDomTree such that they match the CFG.

This will drastically help with #2 because what I have as a local fix using the old DT is something like this:

+   if (DT) {
+    // Check the dom children of each block in loop and if it is outside the
+    // current loop, update it to the preheader.
+    for (auto *BB: L->blocks()) {
+      auto *DomNodeBB = DT->getNode(BB);
+      for (auto *DomChild: DomNodeBB->getChildren()) {
+        if (!L->contains(LI->getLoopFor(DomChild->getBlock()))) {
+          DT->changeImmediateDominator(DomChild, DT->getNode(PreHeader));
+        }    
+      }    
+    }
+   }

This is a more general fix using the old DT to handle the non-immediate successors of an exit block in the loop.

Doesn't this loop have to be run until you reach a fixpoint?

In D56284#1348693, @kuhar wrote:

In D56284#1348681, @anna wrote:

the same bug can happen with:

1. latch exit
2. non-immediate successors of latchexit/otherexit.
   
   I have simplified test cases for each of these and we need a more general fix. Working on this.

... these things are extremely bug-prone; I really do suggest using the automatic updater instead of trying to deal with all tricky corner-cases here :)

the same bug can happen with:

1. latch exit
2. non-immediate successors of latchexit/otherexit.

The IDom can be any block within the loop (as mentioned in the comment).
So, we need to check for IDom *contained* in the loop.
Added test case shows where the IDom is within the inner loop of the original loop.
Both test cases pass now with the fix.

In D56284#1345766, @kuhar wrote:

@anna Can this be rewritten to use the DomTreeUpdater utility?

@kuhar, I have not looked at the DTU uttility yet, but I'd think it can be rewritten (unless there are some limitations for the DomTreeUpdater utility). However, that would be a large enough change and I think it's better to do it as a separate change at a later point rather than as part of fixing this bug. Also, there are enough DT updates through out this code that it will take a while to get through the whole process of porting over for runtime unrolling.

Context and history for runtime unrolling: https://reviews.llvm.org/D35304

LGTM. Thanks for working through the revisions.

Pls add -verify-memoryssa to the RUN command as well, now that MSSA is getting updated.
Comment inline.

LGTM.

There's an LCSSA not being preserved bug in the original landed change (@dmgreen added the test case). As part of fixing it, pls add the following options to your RUN command in the test "-verify-loop-info -verify-dom-info -verify-loop-lcssa". This will make sure all forms of verification are checked.

LGTM. Thanks for working through the comments.

addressed review comments

based on above comments.

Forgot to mark request changes. See previous inline comments.