Page MenuHomePhabricator

[PoC][RISCV][LTO] Pass target-abi via module flag metadata
Needs ReviewPublic

Authored by khchen on Jan 5 2020, 9:42 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary
  1. Support ABI checking with per function target-features

if users don't specific -mattr, the default target-feature come from IR attribute.

  1. Enable LTO for RISCV
  2. add TargetMachine::initTargetOptions to overwirte MCOptions.ABINameaccording to module info

Diff Detail

Event Timeline

khchen created this revision.Jan 5 2020, 9:42 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 5 2020, 9:42 PM
khchen updated this revision to Diff 236296.Jan 5 2020, 9:51 PM
khchen updated this revision to Diff 237084.Jan 9 2020, 8:01 AM
khchen edited the summary of this revision. (Show Details)
evandro added a subscriber: evandro.Jan 9 2020, 8:37 AM

Seems like this patch mixed with LTO related changes? Could you clean it up?

khchen updated this revision to Diff 237114.Jan 9 2020, 9:37 AM

remote LTO related code.
this PoC include D70837 patch for generate correct code.

Hi @efriedma
Could you please guide me and review this PoC?
or take a look at this [[ take a look at | maillist ]] thread?
Thank you!

tejohnson added inline comments.
llvm/lib/LTO/LTOBackend.cpp
151

This is going to be problematic. The Conf is a reference to the Config object saved on the LTO class instance shared by all backend invocations (the regular LTO module if one exists and any ThinLTO modules). They will end up clobbering each other's values here - although from the assert in initTargetOptions I see they are required to all have the same value anyway. Still, it is not good as the assert may actually be missed with unlucky interference between the threads. The Config object here should really be marked const, let me see if I can change that.

You could make a copy of the Config here, but that essentially misses the assertion completely.

A better way to do this would be in LTO::addModule, which is invoked serially to add each Module to the LTO object.

However - this misses other places that invoke createTargetMachine - should other places be looking at this new module flag as well? One example I can think of is the old LTO API (*LTOCodeGenerator.cpp files), used by linkers such as ld64 and some other proprietary linkers and the llvm-lto testing tool. But I have no idea about other invocations of createTargetMachine.

Note that changes to LTO.cpp/LTOBackend.cpp (the new LTO API) needs some kind of llvm-lto2 based test.

lenary added inline comments.Jan 13 2020, 8:21 AM
llvm/lib/LTO/LTOBackend.cpp
151

Thank you for this feedback.

I've been looking at how to add an overridable TargetMachine hook which is not dissimilar to this static function, but is overridable by TargetMachine subclasses. It sounds like this approach will also not work (unless the TargetMachine is allowed to update its (Default)Options in LTO without issue).

I am hoping to get a patch out today for review (which does not include the RISC-V specific parts of this patch, and only includes a default empty implementation), but I imagine it will remain unsatisfactory for LTO for the same reasons this is.

tejohnson added inline comments.Jan 13 2020, 8:34 AM
llvm/lib/LTO/LTOBackend.cpp
151

Presumably you could still do the same thing I'm suggesting here - validate and aggregate the value across modules in LTO::addModule. Then your hook would just check the aggregated setting on the Config.

lenary added inline comments.Jan 13 2020, 8:51 AM
llvm/lib/LTO/LTOBackend.cpp
151

D72624 is the patch I have prepared, noting I haven't had time to implement the aggregation yet, which suggests that patch's approach is too general.

tejohnson added inline comments.Jan 13 2020, 12:28 PM
llvm/lib/LTO/LTOBackend.cpp
151

FYI I committed a change to make the Config object passed down to the backends a const reference in d0aad9f56e1588effa94b15804b098e6307da6b4.

lenary added inline comments.Jan 14 2020, 3:26 AM
llvm/lib/LTO/LTOBackend.cpp
151

Thank you! It is useful to have this restriction explicit :)