Page MenuHomePhabricator

[AMDGPU] Simplify handleAddressTakenFunctions. NFC.
Needs ReviewPublic

Authored by foad on Jun 4 2021, 5:26 AM.

Details

Summary

Use standard depth first iterators to simplify the implementation. Also
don't bother passing in ModuleCG when it is available in the class.

Diff Detail

Event Timeline

foad created this revision.Jun 4 2021, 5:26 AM
foad requested review of this revision.Jun 4 2021, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 5:26 AM
madhur13490 added inline comments.Jun 4 2021, 6:00 AM
llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp
246

Please move this check inside the function otherwise every caller will have to do this check.

madhur13490 added inline comments.Jun 4 2021, 6:03 AM
llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp
234

It is better to keep this assert.

foad updated this revision to Diff 350212.Jun 7 2021, 2:54 AM

Change removeAttributes to take a reference. Reinstate an assert.

foad marked an inline comment as done.Jun 7 2021, 2:58 AM
foad added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp
246

I disagree. "Remove attributes from F" is a simpler API than "remove attributes from F, except if F is null". I have changed removeAttributes to take a reference to make it more obvious that F must be a function.

Also, there is only one caller, and even before my patch there were two callers but only one of them had to cope with F possibly being null. This is caused by a quirk of the CallGraph which contains artificial nodes for external callers/callees, where getFunction returns null.

madhur13490 added inline comments.Jun 24 2021, 12:12 AM
llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp
246

Yes, the quirks of CallGraph makes me worry about this. Please make sure it does not regress as I had to specifically check for !F as a result of failed AOMP smoke tests.

Commit 660cae84c3144a42272daa16415fc9a2532773c4 reverts the base patch due to an unsupported scenario but thankfully this patch fixes that issue. Please rebase and keep the support intact.

Please remove NFC due to the new context. @JonChesterfield found an issue with recursions in the base patch. The following case depicts the scenario

target triple = "amdgcn-amd-amdhsa"
  
define i64 @f(i64 %x)  {
entry:
        br i1 undef, label %return, label %if.end

        if.end:
        %call = call i64 @f(i64 undef)
        unreachable

        return:
        ret i64 1
}

define i8* @unrelated() {
entry:
 ret i8* bitcast (i64 (i64)* @f to i8*)
}

declare amdgpu_kernel void @has_a_kernel(i32)

Please add this test as a part of the patch.