This is an archive of the discontinued LLVM Phabricator instance.

Attach attribute "trap-func-name" to IR function
ClosedPublic

Authored by ahatanak on Jun 29 2015, 9:40 PM.

Details

Summary

This patch is part of the work to make LTO and function multi-versioning work correctly.

Currently, the trap function name users specify on the command line using -ftrap-function is passed to the backend via TargetMachine's TargetOptions. This patch attaches attribute "trap-func-name" to the IR function instead so that we can use -ftrap-function for LTO builds and change the trap function name on a per-function basis.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 28748.Jun 29 2015, 9:40 PM
ahatanak retitled this revision from to Attach attribute "trap-func-name" to IR function.
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 edited edge metadata.Jun 29 2015, 9:42 PM

Not sure we necessarily need a "put these function attributes on the functions" command line and map? Rationale?

-eric

Not sure we necessarily need a "put these function attributes on the functions" command line and map? Rationale?

That's a good point. I don't think we need a new cc1 option for trap-function at all.

ahatanak updated this revision to Diff 28752.Jun 29 2015, 10:35 PM
ahatanak edited edge metadata.

Remove cc1 option and simplify patch.

Thanks, I'll send the corresponding llvm patch shortly.

ahatanak updated this revision to Diff 28837.Jun 30 2015, 6:46 PM

The new patch attaches "trap-func-name" to the call sites instead of the calling functions, which simplifies the handling of the attribute when functions are merged and inlined.

The relevant discussion I had with Eric is here:
http://reviews.llvm.org/D10832

echristo accepted this revision.Jul 1 2015, 10:41 PM
echristo edited edge metadata.

You'll want to also test a trap inside of a ctor/dtor, global constructor (if you can), etc. The general change looks fine though.

Thanks!

-eric

This revision is now accepted and ready to land.Jul 1 2015, 10:41 PM
This revision was automatically updated to reflect the committed changes.

I wasn't sure how to test traps inside ctor/dtor or global constructor, so I went ahead and committed the reviewed patch. I can add the tests later if I can find a way to emit traps for those functions.

pcc added a subscriber: pcc.Feb 14 2018, 6:51 PM

Maybe I'm missing something, but I don't see why we need this attribute. Couldn't clang have been changed to implement -ftrap-function by generating a call to the trap function instead of emitting an llvm.trap intrinsic call?

AMDGPUAnnotateKernelFeatures::addFeatureAttributes looks for an llvm.trap instruction and adds attribute "amdgpu-queue-ptr" to the function if it finds one.

Other than that, it looks like your idea would work too.