Make Descriptor Offsets Relocatable #457

csyonghe · 2020-02-10T21:13:38Z

This PR is based on PR #440.

The commit is based on the original changes made by s-perron
that emits a relocatable entry for each descriptor load, instead
of baking in a constant offset during initial compilation. The
compiler patches up the compiled binary with actual descriptor
offset at a later time when that info is available.

amdvlk-admin · 2020-02-10T21:17:03Z

Can one of the admins verify this patch?

csyonghe · 2020-02-10T21:19:03Z

This branch won't pass CI because it requires GPUOpen-Drivers/llvm-project#1 for llvm-project.

nhaehnle

Thanks for giving us a preview. I already have some detail comments.

More importantly, on a high level, I don't think this patch can go in until we've properly discussed how all of this is going to work in detail. Some things that come to mind:

Who will be responsible for doing the relocation. PAL is quite likely to be preferable over LLPC.
What will the relocation types look like (the ELF part of it).
How do we actually represent the relocatable offsets in LLVM IR. On that one, I think the intrinsic may be misplaced, and using a special global value would be a better solution.

llpc/context/llpcCompiler.cpp

llpc/patch/llpcPatchDescriptorLoad.cpp

llpc/context/llpcCompiler.cpp

s-perron

Considering that you have multiple forms of relocation (samplers, descriptor resource, other), you might want to add a test for each of them.

Otherwise this looks good to me with a few nits.

llpc/context/llpcCompiler.cpp

llpc/patch/llpcPatchDescriptorLoad.cpp

llpc/test/shaderdb/PipelineCs_RelocConst.pipe

llpc/test/shaderdb/PipelineVsFs_RelocConst.pipe

llpc/util/llpcElfWriter.cpp

csyonghe · 2020-03-20T00:12:03Z

@nhaehnle @trenouf This PR is now ready for review. These changes only implement the first step of userDataNode relocation: the offsets of descriptors in a descriptor table. It does not address other issues brought up by @trenouf in #474. Those will come in future PRs.

AMDGPU changes are submitted to Phabricator as in https://reviews.llvm.org/D76440
Those changes are required for this PR to pass CI checks.

JaxLinAMD · 2020-04-13T04:28:56Z

retest this please

JaxLinAMD · 2020-04-14T03:23:59Z

retest this please

JaxLinAMD · 2020-04-14T04:25:27Z

@csyonghe all agents are hanged because of this pull request, please make sure hang is not observed on your local environment when do next force-push.

csyonghe · 2020-04-14T04:26:40Z

@csyonghe all agents are hanged because of this pull request, please make sure hang is not observed on your local environment when do next force-push.

How do I setup my local environment to run the tests?

JaxLinAMD · 2020-04-14T04:39:01Z

@csyonghe , do you mean how to fetch cts source code and compile cts debug version driver?

csyonghe · 2020-04-14T04:41:11Z

I am not sure how to produce the exact same environment as the testing server, including buildling the driver and run it. I can try build the Stadia version and run it on stadia server, but that might differ from the server. I am just wondering if there are some instructions I can follow to setup the testing environment.

JaxLinAMD · 2020-04-14T04:46:50Z

I think you can try on stadia environment if it's more convenient for you. if you want try on local enviroment, you need to setup amdgpu-pro driver and cts binary.

JaxLinAMD · 2020-04-14T04:49:29Z

do you need instructions on building open-source driver or building cts binary?

csyonghe · 2020-04-14T04:51:11Z

I can build the driver. I also have the instructions on building the cts binary. I am just not sure how to deploy and test the driver on a local environment.

JaxLinAMD · 2020-04-14T04:57:37Z

you need to install a opensource driver from repos in README, and replace /usr/lib/x86_64-linux-gnu/amdvlk64.so on ubuntu with drivers you built.

sudo wget -qO - http://repo.radeon.com/amdvlk/apt/debian/amdvlk.gpg.key | sudo apt-key add -
sudo sh -c 'echo deb [arch=amd64] http://repo.radeon.com/amdvlk/apt/debian/ bionic main > /etc/apt/sources.list.d/amdvlk.list'
sudo apt-get remove amdvlk /* If old version is installed on the machine, remove it first */
sudo apt update
sudo apt-get install amdvlk

csyonghe · 2020-04-14T05:21:40Z

I see. Thanks!

amdrexu · 2020-04-14T08:52:52Z

@csyonghe I still see some coding issues with new coding style. Please have a check.
Such as:
pMDNode -> metaNode (we don't use p prefix any more and names are camelcase, avoid acronym)
A* -> A * (this could be done by clang-format, positions of * and & follow LLVM style now)

trenouf · 2020-04-16T18:48:26Z

lgc/patch/llpcPatchDescriptorLoad.cpp

-  descPtr = builder.CreateBitCast(descPtr, builder.getInt32Ty()->getPointerTo(ADDR_SPACE_CONST));
-  descPtr = builder.CreateGEP(builder.getInt32Ty(), descPtr, offset);
-
+  // The LLVM's internal handling of GEP instruction results in a lot of junk code and prevented selection


Re Jax's report that this change causes test agents to hang: I'm a bit suspicious of this edit here, if only because nothing else looks like it should affect normal pipeline compilation. You could try making it conditional on !isa(descPtr), falling back to the old GEP code when it is a ConstantInt, which it will always be for pipeline compilation.

You are right. This is actually wrong when relocation is not used. In normal mode, offset is in dwords not bytes, so it cannot be just an add. I have corrected this issue and trying to test this before making a push. Thanks so much for pointing this out.

Ah right, that would explain the crash. :-)

I don't like the offset being in dwords for one path and bytes for the other path. I'll have a look at changing the normal path to bytes in a separate change.

This commit consists of the original changes made by s-perron that emits a relocatable entry for each descriptor load, instead of baking in a constant offset during initial compilation. The compiler patches up the compiled binary with actual descriptor offset at a later time when that info is available.

csyonghe · 2020-04-17T19:16:32Z

The Jenkins test still haven't returned any result. Can we restart it?

JacobHeAMD · 2020-04-18T09:00:45Z

There is some problem with Jenkins tests. I've already asked infrastructure team to have a look.

JacobHeAMD · 2020-04-18T12:17:26Z

retest this please

csyonghe · 2020-04-20T03:56:34Z

Can you post the details on which case is failing?

trenouf · 2020-04-20T07:41:05Z

It looks to me like it passed CTS on two agents and hung waiting for the third, so I would take that as a pass.

JaxLinAMD · 2020-04-20T07:42:54Z

let me re-trigger the test again.

JaxLinAMD · 2020-04-20T07:43:29Z

retest this please

JaxLinAMD · 2020-04-20T10:11:38Z

merge process is started, please do not force push.

csyonghe requested a review from s-perron February 10, 2020 21:13

nhaehnle requested changes Feb 11, 2020

View reviewed changes

s-perron mentioned this pull request Feb 19, 2020

Handling descriptor offsets as relocations #474

Closed

csyonghe force-pushed the reloc-desc-offset branch 3 times, most recently from 8b5e957 to a9506b6 Compare February 25, 2020 22:16

s-perron reviewed Feb 26, 2020

View reviewed changes

llpc/context/llpcCompiler.cpp Outdated Show resolved Hide resolved

csyonghe force-pushed the reloc-desc-offset branch 9 times, most recently from 392ccf5 to 1bf58c6 Compare March 3, 2020 01:49

csyonghe force-pushed the reloc-desc-offset branch 5 times, most recently from c061fb9 to 0ca554d Compare March 11, 2020 23:29

csyonghe force-pushed the reloc-desc-offset branch 2 times, most recently from 5d6bea0 to e44d5e6 Compare March 17, 2020 22:38

s-perron reviewed Mar 19, 2020

View reviewed changes

csyonghe force-pushed the reloc-desc-offset branch from e44d5e6 to 4a25fbe Compare March 20, 2020 00:06

csyonghe changed the title ~~[WIP] Make Descriptor Offsets Relocatable~~ Make Descriptor Offsets Relocatable Mar 20, 2020

csyonghe force-pushed the reloc-desc-offset branch from 4a25fbe to bd31a4b Compare March 20, 2020 18:04

csyonghe force-pushed the reloc-desc-offset branch 2 times, most recently from 7bc698d to 8ab05b6 Compare April 13, 2020 20:05

trenouf reviewed Apr 16, 2020

View reviewed changes

csyonghe force-pushed the reloc-desc-offset branch 3 times, most recently from d25b863 to be15f43 Compare April 17, 2020 00:23

csyonghe force-pushed the reloc-desc-offset branch from be15f43 to 20f036f Compare April 17, 2020 00:32

JaxLinAMD merged commit 84123da into GPUOpen-Drivers:dev Apr 21, 2020

trenouf mentioned this pull request Apr 21, 2020

Re-format descriptor offset relocation changes #594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Descriptor Offsets Relocatable #457

Make Descriptor Offsets Relocatable #457

csyonghe commented Feb 10, 2020 •

edited

Loading

amdvlk-admin commented Feb 10, 2020

csyonghe commented Feb 10, 2020

nhaehnle left a comment

s-perron left a comment

csyonghe commented Mar 20, 2020 •

edited

Loading

JaxLinAMD commented Apr 13, 2020

JaxLinAMD commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

amdrexu commented Apr 14, 2020

trenouf Apr 16, 2020

csyonghe Apr 16, 2020

trenouf Apr 17, 2020

csyonghe commented Apr 17, 2020

JacobHeAMD commented Apr 18, 2020

JacobHeAMD commented Apr 18, 2020

csyonghe commented Apr 20, 2020

trenouf commented Apr 20, 2020

JaxLinAMD commented Apr 20, 2020

JaxLinAMD commented Apr 20, 2020

JaxLinAMD commented Apr 20, 2020

Make Descriptor Offsets Relocatable #457

Make Descriptor Offsets Relocatable #457

Conversation

csyonghe commented Feb 10, 2020 • edited Loading

amdvlk-admin commented Feb 10, 2020

csyonghe commented Feb 10, 2020

nhaehnle left a comment

Choose a reason for hiding this comment

s-perron left a comment

Choose a reason for hiding this comment

csyonghe commented Mar 20, 2020 • edited Loading

JaxLinAMD commented Apr 13, 2020

JaxLinAMD commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

JaxLinAMD commented Apr 14, 2020

csyonghe commented Apr 14, 2020

amdrexu commented Apr 14, 2020

trenouf Apr 16, 2020

Choose a reason for hiding this comment

csyonghe Apr 16, 2020

Choose a reason for hiding this comment

trenouf Apr 17, 2020

Choose a reason for hiding this comment

csyonghe commented Apr 17, 2020

JacobHeAMD commented Apr 18, 2020

JacobHeAMD commented Apr 18, 2020

csyonghe commented Apr 20, 2020

trenouf commented Apr 20, 2020

JaxLinAMD commented Apr 20, 2020

JaxLinAMD commented Apr 20, 2020

JaxLinAMD commented Apr 20, 2020

csyonghe commented Feb 10, 2020 •

edited

Loading

csyonghe commented Mar 20, 2020 •

edited

Loading