This is an archive of the discontinued LLVM Phabricator instance.

Use function attribute "trap-func-name" and remove TargetOptions::TrapFuncName
ClosedPublic

Authored by ahatanak on Jun 29 2015, 11:16 PM.

Details

Summary

This patch changes normal isel and fast-isel to read the user-defined trap function name from function attribute "trap-func-name" instead of from TargetOptions::TrapFuncName.

This is the llvm side change that goes with this clang patch:
http://reviews.llvm.org/D10831

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 28754.Jun 29 2015, 11:16 PM
ahatanak retitled this revision from to Use function attribute "trap-func-name" and remove TargetOptions::TrapFuncName.
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added reviewers: echristo, dexonsmith.
ahatanak added a subscriber: Unknown Object (MLST).
echristo accepted this revision.Jun 30 2015, 11:55 AM
echristo edited edge metadata.

Couple of small inline comments.

Thanks!

-eric

lib/CodeGen/SelectionDAG/FastISel.cpp
1351

Fix the typo in the comment while you're here? ;)

1353

MF->getFunction() :)

This revision is now accepted and ready to land.Jun 30 2015, 11:55 AM

Thanks.

I realized while reading the review comments that I could attach the attribute to the CallInst instead of the caller function. That way, we can don't have to check that two functions have the same "trap-func-name" value when the functions are inlined or merged.

call void @llvm.trap() #0
...
attributes #0 = { noreturn nounwind "trap-func-name"="foo111" }

Do you see a problem with this approach?

I think that'll cause problems when we merge translation units at module
link time? i.e. we only have one llvm.trap intrinsic.

-eric

I think that'll cause problems when we merge translation units at module
link time? i.e. we only have one llvm.trap intrinsic.

Attaching the attribute to the declaration of llvm.trap would cause problems when linking modules, but attaching it to the call site should be fine. The linked module looks like this:

declare void @llvm.trap() #2
...
call void @llvm.trap() #4
...
call void @llvm.trap() #5
...
attributes #2 = { noreturn nounwind }
attributes #4 = { noreturn nounwind "trap-func-name"="foo112" }
attributes #5 = { noreturn nounwind "trap-func-name"="foo111" }

How would you deal with inlining?

-eric

In the following example, we can just inline test0 into test1 without checking "trap-func-name" on the call-sites, no? If we attach the attribute to the calling functions, then we'll have to check that the merged functions have the same trap function name.

declare void @llvm.trap() #2
...
define void @test1() #0 {

...
call void @llvm.trap() #4
...
call void @test0()

}

define void @test0() #3 {

...
call void @llvm.trap() #5
...

}

attributes #2 = { noreturn nounwind }
attributes #4 = { noreturn nounwind "trap-func-name"="foo112" }
attributes #5 = { noreturn nounwind "trap-func-name"="foo111" }

Right. I mean, neither of them will "win" which may be what we wanted. Or
not. I don't know. :)

(LTO code generation options ftw? :)

Either way, I don't see it as too much of an issue if you want to go this
way instead.

-eric

ahatanak updated this revision to Diff 28834.Jun 30 2015, 6:35 PM
ahatanak edited edge metadata.

Here is my new patch. Sorry for asking for a review again.

  • Changed normal isel and fast isel to read the "trap-func-name" attached to the call-site.
  • Added new functions that are needed to access and modify string attributes attached to CallInst.
  • Made changes to setFunctionAttributes that are needed to override "trap-func-name" in the IR via command line option "trap-func". If the command line option is used, iterate over all instructions in the function and rewrite the function attribute attached to the call sites. Alternatively, I think we can iterate over the list of AttributeSetImpl owned by LLVMContextImpl if visiting all instructions is too expensive.

Seems reasonable to me. One small inline request that you can do or not do as you prefer :)

-eric

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4783–4784

I realize auto is cool and all, but it seems like a bit of overkill here :)

ahatanak updated this revision to Diff 28972.Jul 2 2015, 2:35 PM

Replaced auto with explicit type and removed the line which was causing unused variable warning. Also, changed a line in the test case to check for "trap" but not "trap_llc" or "trap_func_attr".

I'll commit this shortly. Thank you for the review!

This revision was automatically updated to reflect the committed changes.