This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: implement low-level type suitable for MachineInstr selection
ClosedPublic

Authored by t.p.northover on Jul 5 2016, 12:58 PM.

Details

Summary

In my first attempt at a legalization framework for GlobalISel (http://reviews.llvm.org/D21534), Quentin said he intended to move towards a lower-level type representation (essentially just size & lanes). Since the legalization description makes heavy use of types, it's probably best to do that earlier rather than later, hence this patch.

I've added a new LLT class, with 3 variants, designed to look similar to IR-level types:

  • unsized for labels etc
  • sN for scalars & aggregates.
  • <N x sM> for vectors.

At the moment, vectors must contain more than 1 element (so no <1 x double>). I know there are doubts about this being adequate, but I'd like to press on as far as possible under the assumption that RegBankSelect can handle things and it's easier to relax that requirement than impose it later if we're wrong.

There should be no functional change here (we still don't have that much functionality to change!). Test changes are just updating to the new syntax.

Cheers.

Tim.

Diff Detail

Repository
rL LLVM

Event Timeline

t.p.northover retitled this revision from to GlobalISel: implement low-level type suitable for MachineInstr selection.
t.p.northover updated this object.
t.p.northover added a reviewer: qcolombet.
t.p.northover set the repository for this revision to rL LLVM.
t.p.northover added a subscriber: llvm-commits.
t.p.northover edited edge metadata.
qcolombet edited edge metadata.Jul 5 2016, 1:22 PM

Hi Tim,

Looks pretty good to me.
I'd like to keep (at least for now), the type recording capabilities in register bank info though. I believe making a second constructor for LLT that takes SVT is going to be useful anyway.

Cheers,
-Quentin

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
107 ↗(On Diff #62780)

We should be able to keep this functionality by providing a conversion from SVT to LLT, like we do for Type, right?

lib/CodeGen/LowLevelType.cpp
24 ↗(On Diff #62780)

Don't you say we should allow <1 x Ty> kind of vector?
I do not see that here.

30 ↗(On Diff #62780)

One thing to keep in mind is that, at some point, when we will want to do optimizations, we should have a way to know that the padding bits are garbage.
I do not expect this to be handle here, but I should think a way of rebuilding that information like computeKnownBits.

That will probably affect the legalization (expansion) of loads and stores when optimizations are set.

lib/Target/AArch64/AArch64RegisterBankInfo.cpp
193 ↗(On Diff #62780)

I guess we do not need that if we keep the types recording in RegisterBankInfo.

test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir
72 ↗(On Diff #62780)

Thanks for fixing the typo as well, I meant to do it at some point :P.

t.p.northover marked 2 inline comments as done.Jul 5 2016, 2:03 PM

I'd like to keep (at least for now), the type recording capabilities in register bank info though. I believe making a second constructor for LLT that takes SVT is going to be useful anyway.

I removed that reasonably late in development. Going from an SVT to an LLT is fine, the problem was that the only query of the information was based on a MachineInstr's (LLT) type which no longer has the information needed to choose a sane SVT. I suppose we could add some concept of a generic MachineInstr's "realm" (int/float) to provide the dropped info?

Tim.

lib/CodeGen/LowLevelType.cpp
24 ↗(On Diff #62780)

Yep, discovered that myself while integrating this patch with my legalize things. I'll fix it in the updated version.

test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir
72 ↗(On Diff #62780)

Not entirely altruistic: it caused aborts with the new code.

t.p.northover edited edge metadata.
t.p.northover marked 2 inline comments as done.

Only change in the end was enforcing no single-lane vectors in LLT::LLT (I think).

eli.friedman added inline comments.
include/llvm/CodeGen/LowLevelType.h
136

Should this work on a scalar type? (s32 -> <2 x s32>).

qcolombet added inline comments.Jul 7 2016, 12:50 PM
include/llvm/CodeGen/LowLevelType.h
56

Add a comment saying that NumElements must be > 1.

62

brif => brief

66

unsigned instead of int for NumElements and ScalarSizeInBits.

75

const &.
I assume we do not support nullptr.

83

Add a comment saying this should be call only for vector type.
/// \pre isVector() == true.

83

unsigned instead of int.

88

/// \pre isSized() == true.

98

/// \pre isVector() == true.

include/llvm/CodeGen/MachineInstr.h
28 ↗(On Diff #62921)

The ifdef is probably useless.

t.p.northover marked 9 inline comments as done.Jul 7 2016, 1:31 PM
t.p.northover added inline comments.
include/llvm/CodeGen/LowLevelType.h
83

I'll use a normal comment because "\pre" seems to be ignored by our documentation generator (and I prefer it).

136

For symmetry with halfElements, probably. It would make MoreElements also act as "Vectorize" in the same way FewerElements doubles as Scalarize which doesn't seem like a bad thing.

include/llvm/CodeGen/MachineInstr.h
28 ↗(On Diff #62921)

Why? It seems rather weird to #ifdef the use of LLT but not its definition.

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

Fix patch after Quentin & Matt's comments.

Uploaded completely the wrong diff last time. This should fix it to what I intended.

Updating diff. Very minor changes.

qcolombet accepted this revision.Jul 19 2016, 4:21 PM
qcolombet edited edge metadata.

Hi Tim,

LGTM.

Thanks,
-Quentin

This revision is now accepted and ready to land.Jul 19 2016, 4:21 PM

Thanks Quentin, it's r276158.