This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add an AMDGPU pass to promote bitcast calls
ClosedPublic

Authored by scott.linder on Oct 1 2018, 12:03 PM.

Details

Summary

AMDGPU only supports direct calls, but at lower optimization levels it fails to lower statically direct calls which appear indirect due to a bitcast, e.g. calls of the form call ... bitcast (... @func to ...)(...)

Update CallPromotionUtils to handle both bitcast and pointer cast of argument and return types and use it in an AMDGPU pass to promote all possible calls before inlining.

Diff Detail

Repository
rL LLVM

Event Timeline

scott.linder created this revision.Oct 1 2018, 12:03 PM
This revision is now accepted and ready to land.Oct 1 2018, 12:11 PM

@mssimpso Are you OK with reviewing the changes to CallPromotionUtils.cpp in this patch? I wasn't sure who the correct code owner was.

mssimpso accepted this revision.Oct 1 2018, 12:56 PM

@mssimpso Are you OK with reviewing the changes to CallPromotionUtils.cpp in this patch? I wasn't sure who the correct code owner was.

I'm not sure there is a code owner, but the CallPromotionUtils part looks good to me.

@mssimpso Are you OK with reviewing the changes to CallPromotionUtils.cpp in this patch? I wasn't sure who the correct code owner was.

I'm not sure there is a code owner, but the CallPromotionUtils part looks good to me.

Sounds good, thank you for taking a look.

arsenm requested changes to this revision.Oct 1 2018, 7:39 PM

How does this differ from WebAssemblyFixFunctionBitcasts? Should we have a common pass for this?

The generic utils changes are also a separate patch

lib/Target/AMDGPU/AMDGPUFixFunctionBitcasts.cpp
30–32 ↗(On Diff #167793)

This can be dropped

test/CodeGen/AMDGPU/call-constexpr.ll
1 ↗(On Diff #167793)

Should have an opt run of just the pass

This revision now requires changes to proceed.Oct 1 2018, 7:39 PM

The WebAsm code essentially does the same thing, but also creates wrapper functions when the number of arguments differ, or when the return value differs (it seems like this could just be a cast as well). I will move the WebAsm pass to lib/Transform and use it in both backends. I am not sure who from WebAsm to ask to take a look once I'm done?

After reading the WebAsm code more closely it seems like our goals are different. They seem to support indirect calls, but only if certain properties of the functions match (namely number of arguments and return type). For example, the existing pass does not transform anything (e.g. leaves a bitcast function call present) if the bitcast only modifies argument types in a trivial way. For AMDGPU we need to always eliminate the bitcast, and we don't necessarily need to be generating wrapper functions.

@sbc100 I am not sure who to ask, but you touched the WebAssemblyFixFunctionBitcasts pass recently; is my assessment of the intent of the pass accurate?

Your assessment is about right yes. We assembly supports indirect calls but the function signatures have to match in the webassembly type system. In fact the executable won't even validate (won't load) if it contains such invalid calls. So it sounds like the goals are not the same.

Move CallPromotionUtils changes to new review (https://reviews.llvm.org/D52792)

Address feedback, and also remove call to setPreservesCFG as CallPromotionUtils can modify the CFG.

Thanks, Sam. I think the requirements of the two passes are different enough that combining them is not particularly useful.

arsenm added inline comments.Oct 3 2018, 2:12 AM
lib/Target/AMDGPU/AMDGPUFixFunctionBitcasts.cpp
35 ↗(On Diff #167993)

Should use visitCallSite to handle invokes as well

37–38 ↗(On Diff #167993)

How can this happen?

scott.linder marked an inline comment as done.Oct 3 2018, 8:58 AM
scott.linder added inline comments.
lib/Target/AMDGPU/AMDGPUFixFunctionBitcasts.cpp
37–38 ↗(On Diff #167993)

I'm not entirely sure, but the same check is present in CallSite::isIndirectCall() so I assumed there was a reason. Looking at the implementation I don't see how it is possible, so I will remove it.

scott.linder edited the summary of this revision. (Show Details)

Address feedback

Add test for promoting invoke instruction

arsenm accepted this revision.Oct 23 2018, 2:34 PM

LGTM

This revision is now accepted and ready to land.Oct 23 2018, 2:34 PM

Move addPass for new pass

arsenm accepted this revision.Oct 25 2018, 1:21 PM

LGTM

This revision was automatically updated to reflect the committed changes.