This is an archive of the discontinued LLVM Phabricator instance.

[aarch64][globalisel] Move data into <Target>GenRegisterBankInfo. NFC.
ClosedPublic

Authored by dsanders on Dec 20 2016, 2:51 AM.

Event Timeline

dsanders updated this revision to Diff 82080.Dec 20 2016, 2:51 AM
dsanders retitled this revision from to [aarch64][globalisel] Move data into <Target>GenRegisterBankInfo. NFC..
dsanders updated this object.
dsanders added reviewers: qcolombet, t.p.northover, ab, rovka.
dsanders added a subscriber: llvm-commits.
qcolombet accepted this revision.Dec 20 2016, 10:00 AM
qcolombet edited edge metadata.

Hi Daniel,

LGTM modulo a couple of nitpicks.

Cheers,
-Quentin

lib/Target/AArch64/AArch64GenRegisterBankInfo.def
99

I would rather have the register banks in the AArch64 namespace. I expect we will use them directly, and having to specify AArch64GenRegisterBankInfo seems painful.

lib/Target/AArch64/AArch64RegisterBankInfo.h
78

I would prefer that we call this checkXXX and return a bool.
The assert is then part of the caller, i.e., assert(checkXXX).

78

const std::vector<...> &

This revision is now accepted and ready to land.Dec 20 2016, 10:00 AM
qcolombet added inline comments.Dec 20 2016, 11:47 AM
lib/Target/AArch64/AArch64RegisterBankInfo.h
32

FWIW, I believe that generating that part could be part of the tablegen switch. I indeed don't think it makes much to have a XXXGen manually written. I.e., could be squashed with the tablegen switch.

But I am fine with the intermediate step if it does not last long.

dsanders added inline comments.Dec 21 2016, 8:46 AM
lib/Target/AArch64/AArch64GenRegisterBankInfo.def
99

Hmm, I can move it to llvm::AArch64 but I have to either hard-code the number of register classes and eliminate finishInit() or allow these variables to be in a partially-initialized state until the TargetRegisterInfo exists and something (currently <Target>RegisterBankInfo) can finish the initialization. If we do this after the tablegen patch then we can have tablegen emit the correct size.

Which would you prefer?

Also, just so we're on the same page: Which passes do you expect to use them directly?

lib/Target/AArch64/AArch64RegisterBankInfo.h
32

It could be part of the tablegen switch. The reason it isn't is that Ahmed suggested adding the class first and then having a tablegen patch make tablegen emit that same implementation.

78

I would prefer that we call this checkXXX and return a bool.
The assert is then part of the caller, i.e., assert(checkXXX).

Ok.

const std::vector<...> &

Well spotted

qcolombet added inline comments.Jan 3 2017, 1:24 PM
lib/Target/AArch64/AArch64GenRegisterBankInfo.def
99

If we do this after the tablegen patch then we can have tablegen emit the correct size.

That would be my preference.

Which passes do you expect to use them directly?

I expect some AArch64 specific passes to use them directly like what we do with GPR64RegClass and such. Anyway, it would be strange if the register classes and the register banks are in different namespace IMHO.

lib/Target/AArch64/AArch64RegisterBankInfo.h
32

Up to you.
I would say that either way is fine as long as XXXGenRegisterBankInfo gets generated by TableGen shortly.

dsanders updated this revision to Diff 83603.Jan 9 2017, 3:06 AM
dsanders edited edge metadata.

Rebased onto new D27809.

Also, the *RegBank globals are back in the llvm::<Target> namespace and
assertPartialMappingIdx() is now checkPartialMappingIdx() as requested.

dsanders added inline comments.Jan 9 2017, 3:26 AM
lib/Target/AArch64/AArch64RegisterBankInfo.h
78

I haven't made the change to a const std::vector<...> & because the arguments are initializer_lists and cannot be bound to the reference type. I could fix this but it makes the caller side a bit uglier:

std::vector<PartialMappingIdx> Temp = {PMI_GPR32, PMI_GPR64};
checkPartialMappingIdx(..., Temp);

and only saves us a small constant amount of time in debug builds.

Do you think I should leave it as-is or go with the uglier code?

ab accepted this revision.Jan 9 2017, 1:53 PM
ab edited edge metadata.

This LGTM, but I'll let Quentin have final say.

lib/Target/AArch64/AArch64RegisterBankInfo.h
32

It could be part of the tablegen switch. The reason it isn't is that Ahmed suggested adding the class first and then having a tablegen patch make tablegen emit that same implementation.

Sorry if I was unclear: I think the tablegen patch should be NFC (the main issue being PartialMapping), but not necessarily that it generates exactly the same code as existed before. So, I'd also say squash this with the tablegen switch, but it's not a big deal either way.

78

What about ArrayRef<PartialMappingIdx> ?

LGTM as well.

Ahmed's suggestion to use ArrayRef is probably best.

dsanders added inline comments.Jan 10 2017, 8:16 AM
lib/Target/AArch64/AArch64RegisterBankInfo.h
78

That worked nicely. Thanks

dsanders updated this revision to Diff 83812.Jan 10 2017, 8:16 AM
dsanders edited edge metadata.

Use ArrayRef

dsanders updated this revision to Diff 84265.Jan 13 2017, 2:35 AM

Refresh before commit

dsanders closed this revision.Jan 13 2017, 3:04 AM
This revision was automatically updated to reflect the committed changes.