Page MenuHomePhabricator

GlobalISel: legalization policy class
ClosedPublic

Authored by t.p.northover on Jul 6 2016, 4:47 PM.

Details

Reviewers
qcolombet
arsenm
Summary

Hi,

As suggested in D21534, I've split off the class deciding on how legalization should proceed (plus the legalizeInstr stub) into a separate unit-tested patch.

I've addressed most of the relevant comments from the original diff, with two outstanding issues:

  • We'll need a richer setAction interface to cope with multiple types on instructions.
  • Similarly, we'll need to be able to cope with per-address-space legalization.

I fully agree with both of these, but think they'll probably be added organically as we grow (certainly the first is needed for AArch64, and I'll make sure the second doesn't get neglected).

As with most of the other GlobalISel things, it's pretty rough around the edges as a prototype and I'm hoping to fill in the details as-needed and as-testable.

Tim.

Diff Detail

Repository
rL LLVM

Event Timeline

t.p.northover retitled this revision from to GlobalISel: legalization policy class.
t.p.northover updated this object.
t.p.northover added reviewers: arsenm, qcolombet.
t.p.northover set the repository for this revision to rL LLVM.
t.p.northover added a subscriber: llvm-commits.

Are you sure you uploaded the right patch here? This isn't against trunk, as far as I can tell.

Ah sorry, it does depend on D22008. I did mean to mention that but forgot. Together they should apply cleanly.

arsenm added inline comments.Jul 7 2016, 11:38 AM
include/llvm/CodeGen/GlobalISel/MachineLegalizer.h
68

There's one value left in uint8_t, can we make it be unsupported? We custom lower a variety of operations now just to emit an error saying they don't work

lib/CodeGen/GlobalISel/MachineLegalizer.cpp
81–82

No else after return

t.p.northover marked an inline comment as done.

Add Unsupported class for error reporting.

t.p.northover marked an inline comment as done.Jul 7 2016, 12:08 PM
t.p.northover added inline comments.
include/llvm/CodeGen/GlobalISel/MachineLegalizer.h
68

Bother, I hadn't ignored you last time but that seems to have ended up in the wrong patch. I'll relocate it.

Sorry, wrong diff. And I think I addressed Eli's comment too.

Sorry, complete mess-up with the last revision. I don't think anything substantial has changed here yet I'm just trying to undo my last upload.

Ping? I'm getting patches stacking on top of this (mostly straightforward themselves, but difficult to keep rebasing and making progress while the low-level bits are unsettled).

qcolombet added inline comments.Jul 18 2016, 5:12 PM
include/llvm/CodeGen/GlobalISel/MachineLegalizer.h
84

I found the returns comment hard to understand :).
I believe that means MI has been legalized successfully?
What happen when we return false? In particular is the function left untouched as nothing as happened or are we in a strange state that we may not be able to recover?

86

ancillary :).

89

Shouldn’t we hide this function?

For instance, I would have expected that the class call it internally the first time it does a query.
Note: I understand why for compile time reason we would like to expose this API. I am just nervous that if we forget to do it, we end up with a bunch of hard to understand errors.
Alternative, could you add an assert of some sort catch that?

110

Those are the queries you were talking about in computeTables, right?

include/llvm/CodeGen/LowLevelType.h
88

This diff shouldn’t be in that patch, right?

t.p.northover marked 3 inline comments as done.Jul 19 2016, 10:23 AM

Updated diff coming soon (I wish you could both answer and upload a patch at the same time).

include/llvm/CodeGen/GlobalISel/MachineLegalizer.h
84

The intent is that legalizeInstr will always succeed. False indicates the instruction was already legal (and so no changes were made). I'll clarify the wording.

86

You know, I thought so too! But Matt quite tactfully pointed out that I'd been mistaken all my life.

89

I'm not too worried about the errors, it's going to be once-per-backend which is pretty miniscule. I'll put an assert in I think.

110

Yep.

include/llvm/CodeGen/LowLevelType.h
88

Probably the previous one (though It think this might be the first use of that function?). I'll move it.

t.p.northover marked 3 inline comments as done.

Updating diff with latest comments.

qcolombet accepted this revision.Jul 19 2016, 6:59 PM
qcolombet edited edge metadata.

LGTM.

include/llvm/CodeGen/GlobalISel/MachineLegalizer.h
87

:D

111

Could you mention that in a comment?

This revision is now accepted and ready to land.Jul 19 2016, 6:59 PM
t.p.northover closed this revision.Jul 20 2016, 2:21 PM

Thanks Quentin. Committed as r276184.