search

GPUOpen-Drivers / llpc

LLVM-Based Pipeline Compiler
MIT License
166 stars 115 forks source link

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

Closed ruiminzhao closed 9 months ago

ruiminzhao commented 9 months ago

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 commented 9 months ago

Test summary for commit 284edc5b555708d7687cf9e15aa9d6bd660c65d4

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 commented 9 months ago

Test summary for commit ccac591f6d9d568fe59bf8b9d0d5162e2a5f8214

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 commented 9 months ago

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?

ruiling commented 9 months ago

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 commented 9 months ago

Close this PR and new PR submitted which only added the assertion in https://github.com/GPUOpen-Drivers/llpc/pull/2881.

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

Thanks.