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

[kosarev]

Summary

The intended convention seems to be that the intervals do not include the
value of the end point.

[foad]

Nak without more explanation. What does this fix?

[kosarev]

Update the commit description.

[foad]

Is this NFC? So long as the two pieces of code you updated are consistent, there is no bug, is there?

[kosarev]

Is this NFC? So long as the two pieces of code you updated are consistent, there is no bug, is there?

What is the SGPR interval change consistent with?

For VGPRs, I guess seeing it an NFC rather than a couple bugs masking one another would imply being fine with different instances of the same type having different invariants or maybe disregarding the conceptual dimension of it all whatsoever, if this is a better crime.

[foad]

NFC is not about "conceptual dimensions", it is about whether (to the best of your understanding) this patch will change the behaviour of the compiler.

[kosarev]

Jay, do you suggest this should be marked NFC?

[foad]

In D157089#4565353, @kosarev wrote:

Jay, do you suggest this should be marked NFC?

Yes, because it looks to me like it does not change any behaviour. But I really wanted to understand whether it was *intended* to change any behaviour.

[kosarev]

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.

[foad]

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.

OK. I was mostly looking at the current upstream code, where RegisterEncoding does not use RegIntervals. Having looked more closely at what you did in D157088, I think I understand your point of view.

I would have found it less confusing if you had swapped the order of these two patches, or even squashed them together, but it does not really matter now.

[kosarev]

Rebased on the current top of tree. Remove dependency on D157088.

[kosarev]

@foad Jay, does this still LGTY after the rework?

[foad]

Yes it looks fine but I would not call it a "fix" since there is no bug here. (The rationale you gave before: "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" no longer applies, since this is no longer based on D157088.)

[kosarev]

Yes it looks fine but I would not call it a "fix" since there is no bug here.

Right. There still seems to be a problem with testing against the SGPR range, however, with SQ_MAX_PGM_SGPRS being used as the end of the range where in fact it seems to be the size of the SGPR interval (and Encoding.SGPRL never used). Then changing it to assert(Reg >= Encoding.SGPR0 && Reg <= Encoding.SGPRL); doesn't work because getAddressableNumSGPRs() doesn't seem to include EXEC, which it seems was the expectation initially.

I'm going to update this once again to fix that, and then let's see if using RegUnits can be a better alternative to the intervals as you suggested elsewhere.

llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp

RegInterval WaitcntBrackets::getRegInterval(const MachineInstr *MI,
   // A partial write is not a WAW.
   assert(!Op.getSubReg() || !Op.isUndef());
 
   RegInterval Result;
 
   unsigned Reg = TRI->getEncodingValue(AMDGPU::getMCReg(Op.getReg(), *ST));
 
   if (TRI->isVectorRegister(*MRI, Op.getReg())) {
-    assert(Reg >= Encoding.VGPR0 && Reg <= Encoding.VGPRL);
+    assert(Reg >= Encoding.VGPR0 && Reg < Encoding.VGPRL);
     Result.first = Reg - Encoding.VGPR0;
     if (TRI->isAGPR(*MRI, Op.getReg()))
       Result.first += AGPR_OFFSET;
     assert(Result.first >= 0 && Result.first < SQ_MAX_PGM_VGPRS);
   } else if (TRI->isSGPRReg(*MRI, Op.getReg())) {
     assert(Reg >= Encoding.SGPR0 && Reg < SQ_MAX_PGM_SGPRS);
     Result.first = Reg - Encoding.SGPR0 + NUM_ALL_VGPRS;
     assert(Result.first >= NUM_ALL_VGPRS &&
bool SIInsertWaitcnts::runOnMachineFunction(MachineFunction &MF) {
 
   unsigned NumVGPRsMax = ST->getAddressableNumVGPRs();
   unsigned NumSGPRsMax = ST->getAddressableNumSGPRs();
   assert(NumVGPRsMax <= SQ_MAX_PGM_VGPRS);
   assert(NumSGPRsMax <= SQ_MAX_PGM_SGPRS);
 
   RegisterEncoding Encoding = {};
   Encoding.VGPR0 = TRI->getEncodingValue(AMDGPU::VGPR0);
-  Encoding.VGPRL = Encoding.VGPR0 + NumVGPRsMax - 1;
+  Encoding.VGPRL = Encoding.VGPR0 + NumVGPRsMax;
   Encoding.SGPR0 = TRI->getEncodingValue(AMDGPU::SGPR0);
-  Encoding.SGPRL = Encoding.SGPR0 + NumSGPRsMax - 1;
+  Encoding.SGPRL = Encoding.SGPR0 + NumSGPRsMax;
 
   TrackedWaitcntSet.clear();
   BlockInfos.clear();
   bool Modified = false;
 
   if (!MFI->isEntryFunction()) {
     // Wait for any outstanding memory operations that the input registers may
     // depend on. We can't track them and it's better to do the wait after the