This is an archive of the discontinued LLVM Phabricator instance.

[IRLinker] Replace CallInstr with CallBase
ClosedPublic

Authored by gulfem on Jan 13 2023, 5:07 PM.

Details

Summary

This patch replaces CallInstr with CallBase to cover InvokeInstr
besides CallInstr while removing nocallback attribute on a call site.
It also extends drop-attribute.ll test to include a case for an invoke
instruction.

Diff Detail

Event Timeline

gulfem created this revision.Jan 13 2023, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 5:07 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
gulfem requested review of this revision.Jan 13 2023, 5:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 5:07 PM
arsenm added a subscriber: arsenm.Jan 13 2023, 7:31 PM

Needs test

Needs test

What kind of test would be good to add here?
I had a test (drop-attribute.ll) in D137360, that checks the usage of updateAttributes functionality. Do you suggest extending that for invoke instruction?

Needs test

What kind of test would be good to add here?
I had a test (drop-attribute.ll) in D137360, that checks the usage of updateAttributes functionality. Do you suggest extending that for invoke instruction?

I think you could just extend that test to include an invoke instruction and confirm it is handled correctly.

gulfem updated this revision to Diff 490343.Jan 18 2023, 6:09 PM

Extend drop-attribute.ll test to include a case for invoke instruction.

gulfem updated this revision to Diff 490344.Jan 18 2023, 6:12 PM

Fixed the typo in the comment.

gulfem edited the summary of this revision. (Show Details)Jan 18 2023, 6:13 PM
gulfem edited the summary of this revision. (Show Details)
tejohnson accepted this revision.Jan 18 2023, 6:21 PM

lgtm with one test fix noted below. Thanks for the fix!

llvm/test/Linker/drop-attribute.ll
14

I think you need to add the {{$}} at the end to make sure the nocallback attribute is gone.

This revision is now accepted and ready to land.Jan 18 2023, 6:21 PM
gulfem updated this revision to Diff 490586.Jan 19 2023, 10:26 AM

Addressed tejohnson's feedback.

gulfem marked an inline comment as done.Jan 19 2023, 10:27 AM
This revision was landed with ongoing or failed builds.Jan 19 2023, 10:28 AM
This revision was automatically updated to reflect the committed changes.