[PATCH] D157089: [AMDGPU] Fix dealing with register interval endpoints in SIInsertWaitcnts.

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added a comment.

> But I really wanted to understand whether it was *intended* to change any behaviour.

This fixes incorrect code and so was the intention. The original code is known to be incorrect because it violates use conventions for `RegInterval` as a concept, which in turn we know from its other uses, of which there are many.

In my view, being a fix, this cannot be an NFC. I also think that there is little value in defining NFC as a change that doesn't cause observable behaviour changes purely because there happened to be no uses yet that would otherwise expose the difference.

Consider this:

   int multiply(int a, int b) {
  -    return a + b;
  +    return a * b;
   }
  
   ...
   
   // Currently the only use of multiply().
   return multiply(2, 2);

I see this a functional change, and in my opinion that does not and should not depend on what other uses `multiply()` may have at the time of, say, submitting such a patch. That also does not and should not change anything in whether the original code should be seen correct.

This is solely to explain my views. I have no intention to insist on anything in terms of NFC marks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157089/new/

https://reviews.llvm.org/D157089