PatchBufferOp issues when lowering load ptr(7), ptr(7)

[ruiminzhao]

##Issue Description
There are issue when lowering on load instruction which both result type and pointer operand type are fat buffer pointer.

An example IR:

....
  %6 = call ptr addrspace(7) @lgc.buffer.desc.to.ptr(<4 x i32> %5)

  %10 = load ptr addrspace(7), ptr addrspace(7) %6, align 32
  %11 = load i8, ptr addrspace(7) %10, align 1
  store i8 %11, ptr addrspace(7) %9, align 1

Currently, as %10 and %11 which operand address space are 7, so they are all be added into postVisitLoad list.
The 1st issue is when call replaceLoadStore for %10, it will be lowered into i160 not <4xi32, i32> finally.
Then call replaceLoadStore for %11, it will be crashed when call m_typeLowering.getValue(pointerOperand).

The 2nd issue is TypeLowering.visitLoad will handle on %10 firstly, then BufferOp.postVisitLoad will handle on %10 again
as the pointer operand address space is 7(fat_buffer_pointer), then %10 will be added into m_instructionsToErase again, then it will be crashed when erase this load instruction.

##Issue Solution:
Now I have moved the instructions formatted "load ptr7, ptr7" from typeLowering.visitLoad into PatchBufferOp.postVisitLoad, but I'm not sure this will be correct for other parts such as CPS, which doesn't have postVisitLoad function to handle operand = ptr<7> cases.

Any better solution will be appreciated.

[amdvlk-admin]

Test summary for commit 284edc5

CTS tests (Failed: 0/138443)

• Built with version 1.3.5.2
Ubuntu navi3x, Srdcvk
• Passed: 35211/69228 (50.9%)
• Failed: 0/69228 (0.0%)
• Not Supported: 34017/69228 (49.1%)
• Warnings: 0/69228 (0.0%)
Ubuntu navi2x, Srdcvk
• Passed: 35241/69215 (50.9%)
• Failed: 0/69215 (0.0%)
• Not Supported: 33974/69215 (49.1%)
• Warnings: 0/69215 (0.0%)

[amdvlk-admin]

Test summary for commit ccac591

CTS tests (Failed: 0/138378)

• Built with version 1.3.5.2
Ubuntu navi3x, Srdcvk
• Passed: 35162/69163 (50.8%)
• Failed: 0/69163 (0.0%)
• Not Supported: 34001/69163 (49.2%)
• Warnings: 0/69163 (0.0%)
Ubuntu navi2x, Srdcvk
• Passed: 35241/69215 (50.9%)
• Failed: 0/69215 (0.0%)
• Not Supported: 33974/69215 (49.1%)
• Warnings: 0/69215 (0.0%)

[nhaehnle]

I'll take a look, but how is it possible for you to end up with such code in the first place? What's the corresponding SPIR-V code that would cause you to load or store a buffer pointer in a buffer?

[nhaehnle]

Okay, we need to understand better whether and why this is happening to support this. A couple of strategies:

One idea is to scan the entire code multiple times, with the BufferOpLowering visit of load and store happening during the second scan. But that is bad because the second scan is usually not required and it costs compile time.

Another idea is to teach the visitor infrastructure about potentially having to re-visit newly inserted instructions. Basically, TypeLowering::visitLoad would return some indication that the code which has just been created by it has to be visited again. This would mean that visitor functions can return some llvm_dialects::VisitResult value.

Cc @JanRehders-AMD

[nhaehnle]

We definitely need to find a way that avoids this special case.

[ruiling]

While the TypeLowering::visitLoad is useful in general case. But it does not quite well with PatchBufferOp for such tricky case. Maybe we can just derive BufferOpLowering from TypeLowering, and only register BufferOpLowering::visitLoad without TypeLowering::visitLoad. Can we? The load of fat pointer from memory pointed by fat pointer would be handled solely by BufferOpLowering::postVisitLoad which is just like the idea in the change.

[ruiling]

A more proper way might be to explicitly abort on such kind of complex case in TypeLowering::visitLoad and leave it to other visitor to handle it.

--- a/compilerutils/lib/TypeLowering.cpp
+++ b/compilerutils/lib/TypeLowering.cpp
@@ -488,6 +488,13 @@ void TypeLowering::visitLoad(LoadInst &load) {
   if (types.size() == 1 && types[0] == load.getType())
     return;

+  auto *pointerType = load.getPointerOperand()->getType();
+  auto pointerTypes = convertType(pointerType);
+  // This is somewhat complex that we are lowering both the pointer and the loaded value, skip this to let other visitor
+  // handle it more properly.
+  if (pointerTypes[0] != pointerType)
+    return;
+
   m_builder.SetInsertPoint(&load);

   Type *loadType;

[ruiling]

Another idea is to teach the visitor infrastructure about potentially having to re-visit newly inserted instructions. Basically, TypeLowering::visitLoad would return some indication that the code which has just been created by it has to be visited again. This would mean that visitor functions can return some llvm_dialects::VisitResult value.

Besides revisiting newly inserted instructions, we also need a way to notify other visitors stop visiting the old instruction which was just being lowered. For this case, the TypeLowering::visitLoad needs a way to notify BufferOpLowering: the load instruction has been lowered and marked for deletion, please don't visit/lower it again. I am not sure whether this can be achieved somehow and whether this is doable.

[ruiminzhao]

Close this PR and new PR submitted which only added the assertion in #2881.

Any case meet this pattern: "load ptr(7), ptr(7)", I will reopen this PR for fix.

Thanks.