Page MenuHomePhabricator

AMDGPU/SI: Don't reserve FLAT_SCRATCH on non-HSA targets
ClosedPublic

Authored by mareko on Nov 27 2016, 11:12 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mareko updated this revision to Diff 79356.Nov 27 2016, 11:12 AM
mareko retitled this revision from to AMDGPU/SI: Don't reserve FLAT_SCRATCH on non-HSA targets.
mareko updated this object.
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
397–398 ↗(On Diff #79356)

There should be a query in SIMachineFunctionInfo to check if flat scratch will be used, because it's not even always required for hsa depending on the program.

mareko added inline comments.Nov 28 2016, 9:11 AM
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
397–398 ↗(On Diff #79356)

How to know if flat scratch will be used by HSA? Or can that be done in a later patch?

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
397–398 ↗(On Diff #79356)

I think the query should be added for this patch, but it can just return ST.hasHsaOS(). The smarter logic can be added in a later patch.

mareko updated this revision to Diff 79452.Nov 28 2016, 1:54 PM
mareko edited edge metadata.

Update: Add SIMachineFunctionInfo::flatUsesScratch()

hasStackObjects is a good enough approximation for now. Ideally it would be enabled only if it has an alloca that is addrspacecasted or has a captured private pointer. hasStackObjects would ideally only be alloca derived stack objects, not anything inserted by lowering (which should exist by the time reserved registers are frozen)

hasStackObjects is a good enough approximation for now. Ideally it would be enabled only if it has an alloca that is addrspacecasted or has a captured private pointer. hasStackObjects would ideally only be alloca derived stack objects, not anything inserted by lowering (which should exist by the time reserved registers are frozen)

hasStackObjects doesn't say that scratch is accessed by flat instructions. The user may still want to use a buffer descriptor. Also, I wouldn't like to change the behavior for HSA in this patch (i.e. depend on hasStackObjects), because that needs lit tests adjustments and maybe even more tests.

hasStackObjects is a good enough approximation for now. Ideally it would be enabled only if it has an alloca that is addrspacecasted or has a captured private pointer. hasStackObjects would ideally only be alloca derived stack objects, not anything inserted by lowering (which should exist by the time reserved registers are frozen)

hasStackObjects doesn't say that scratch is accessed by flat instructions. The user may still want to use a buffer descriptor. Also, I wouldn't like to change the behavior for HSA in this patch (i.e. depend on hasStackObjects), because that needs lit tests adjustments and maybe even more tests.

That's why it's an approximation. If there are no stack objects, then obviously none are accessed through flat instructions. I don't think accessing the stack through a user buffer descriptor should be supported

mareko updated this revision to Diff 79818.Nov 30 2016, 2:33 PM
mareko edited edge metadata.

Update: Use FlatScratchInit, also reserve FLAT_SCRATCH on CI.

tstellarAMD accepted this revision.Dec 8 2016, 10:30 AM
tstellarAMD edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Dec 8 2016, 10:30 AM
This revision was automatically updated to reflect the committed changes.