This is an archive of the discontinued LLVM Phabricator instance.

[tablegen][globalisel] Convert the SelectionDAG importer to a tree walking approach. NFC
ClosedPublic

Authored by dsanders on Mar 2 2017, 7:31 AM.

Details

Summary

But don't actually inspect the tree any deeper than we already do. This
change is NFC but the next one will enable full traversal of the
source/destination patterns.

Depends on D30535

Event Timeline

dsanders created this revision.Mar 2 2017, 7:31 AM
dsanders updated this revision to Diff 92919.Mar 24 2017, 4:26 AM

Refresh and ping. This patch is the main bottleneck for a lot of the tablegen work.

ab edited edge metadata.Mar 27 2017, 11:23 AM

This generally seems sensible, but if feasible it might be a good idea to replace the 'Expected<bool>' with 'Error'.

utils/TableGen/GlobalISelEmitter.cpp
1259–1262

Here and elsewhere: is it possible to fold the call into the if? Something like:

if (auto Err = importRulePredicates(M, P.getPredicates()->getValues()).takeError())
  return Err;

That also lets you avoid the not-very-descriptive 'TrueOrError'.

More generally, an 'Expected<bool>' that's always true seems odd. If you don't have plans to add more information, maybe just return an Error? Then the whole takeError vs weird-name problem goes away.

dsanders added inline comments.Mar 28 2017, 7:05 AM
utils/TableGen/GlobalISelEmitter.cpp
1259–1262

Here and elsewhere: is it possible to fold the call into the if? Something like:

if (auto Err = importRulePredicates(M, P.getPredicates()->getValues()).takeError())

return Err;

That also lets you avoid the not-very-descriptive 'TrueOrError'.

I'll make this change in the commit.

More generally, an 'Expected<bool>' that's always true seems odd.

The bool is only there because Expected<void> isn't allowed. Optional<Error> is a bit more accurate but it has a different API and you then have to remember which functions return Expected<X> and which return Optional<Error>. I decided it was best to lean towards consistency.

If you don't have plans to add more information, maybe just return an Error? Then the
whole takeError vs weird-name problem goes away.

If a function always returns an Error, what does it return on success? It would be strange to have an Error that is not an error.

dsanders updated this revision to Diff 93238.Mar 28 2017, 7:07 AM

Fold away the TrueOrError variables.

This revision was automatically updated to reflect the committed changes.
ab added a comment.Mar 29 2017, 9:24 AM

If a function always returns an Error, what does it return on success? It would be strange to have an Error that is not an error.

It returns Error::success(). The name is somewhat awkward, but think of it as a replacement for the old-style 'int' return values, or c++ std::error_code. There are a few examples in http://llvm.org/docs/ProgrammersManual.html#recoverable-errors.

In D30536#713232, @ab wrote:

If a function always returns an Error, what does it return on success? It would be strange to have an Error that is not an error.

It returns Error::success(). The name is somewhat awkward, but think of it as a replacement for the old-style 'int' return values, or c++ std::error_code. There are a few examples in http://llvm.org/docs/ProgrammersManual.html#recoverable-errors.

Hmm, I'm not keen returning an Error that represents success or using multiple error handling strategies for one family of functions. That said, the former is an established idiom and I suppose I could deal with the latter by renaming to createAndImport*() for the ones that return an Expected<X> and keeping import*() for the ones that return Error.

I'll commit a follow-up tomorrow to make that change.