Adds a RegisterBank tablegen class that can be used to declare the register
banks and an associated tablegen pass to generate the necessary code.
Details
- Reviewers
qcolombet rovka t.p.northover ab - Commits
- rGd64d5024a4b8: Re-commit: [globalisel] Tablegen-erate current Register Bank Information
rG517b61cb69d6: Re-commit: [globalisel] Tablegen-erate current Register Bank Information
rGab8194def0e3: [globalisel] Tablegen-erate current Register Bank Information
rL292478: Re-commit: [globalisel] Tablegen-erate current Register Bank Information
rL292367: Re-commit: [globalisel] Tablegen-erate current Register Bank Information
rL292132: [globalisel] Tablegen-erate current Register Bank Information
Diff Detail
- Repository
- rL LLVM
Event Timeline
Small correction to fix a use-after-free that only manifested when asan was disabled.
Just a few nitpicks, other than that LGTM (but you should probably wait for someone more experienced to approve).
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
19 ↗ | (On Diff #80417) | I think it's nicer to use a macro that describes what we're trying to include rather than where we're including it, e.g. GET_REGBANK_DECLS or something of the sort. This also seems to be the style used in GenRegisterInfo.inc, GenSubtargetInfo.inc etc. |
utils/TableGen/RegisterBankEmitter.cpp | ||
202 ↗ | (On Diff #80417) | How about visitRegisterBankClasses? The comment is clear enough on what something like that would mean :) |
233 ↗ | (On Diff #80417) | I think it would be cleaner to generate a comment instead of a moot assertion if Begin == End or Begin + 1 == End (and then you can also remove the I != End check in the loop). |
304 ↗ | (On Diff #80417) | Are the calls to .str() really necessary? |
309 ↗ | (On Diff #80417) | " not first " |
Updated following review:
- Use more descriptive macros when #include'ing tablegen-erated files
- Rename visitor function
- assert Begin!=End and simplify related for-loop.
- Remove redundant .str()'s
- getEnumeratorName() no longer returns a reference to an implicitly constructed local StringRef. This showed up when removing the .str()'s
- last -> first
Thanks. I've updated the patch.
utils/TableGen/RegisterBankEmitter.cpp | ||
---|---|---|
202 ↗ | (On Diff #80417) | As you can tell, I struggled to name this one :-). It doesn't really fit with 'overlapping', 'subclasses', 'subsets', etc. I'll go with visitRegisterBankClasses() |
233 ↗ | (On Diff #80417) | The caller filters out the Begin == End case on line 303 (since Bank.hasPartialMappings() is a test for Begin != End). I think it's good to explicitly assert that here which leads to the same simplification you suggest. Is that ok? |
304 ↗ | (On Diff #80417) | I thought I needed them for type-conversion but apparently not. I've gone through the other .str()'s too. |
Thanks, I like the names a lot better now.
utils/TableGen/RegisterBankEmitter.cpp | ||
---|---|---|
233 ↗ | (On Diff #80417) | Right, good point :) |
This seems reasonable, but:
- I don't think the PartialMapping exclusion mechanism is necessary. What about adding the missing mappings manually before landing this patch? Alternatively, replace the manual code with a copy of the generated mappings (once you have those) before any tablegen changes? That lets us split the generalization from the NFC tablegen changes without requiring hacks in-between.
- I'm not sure it's useful to generate the asserts: I think the intent was to check 1) the correctness of the manual description, and 2) it being consistent with the eventual tablegen output. Have you considered keeping them in AArch64RegisterBankInfo? I suppose generating them lets all targets have them. @qcolombet: what do you think?
- Also, can you extract the RegisterBank code to a shared location? (CodeGenRegBank? maybe rename it to CodeGenRegisters) ? That'll let us reuse it from the selector emitter backend. We can do that separately though.
utils/TableGen/RegisterBankEmitter.cpp | ||
---|---|---|
343 ↗ | (On Diff #80593) | <Record *> |
utils/TableGen/RegisterBankEmitter.cpp | ||
---|---|---|
362 ↗ | (On Diff #80593) | emitSourceFileHeader ? |
It should be easy enough to add FPR's 8 and 16 mapping and CCR's 32 mapping. I'm not sure about FPR's 192 and 384 mappings though. There's definitely some code that assumes the sizes are a power of 2 (e.g. getRegBankBaseIdxOffset()) but I'm not sure how widespread that assumption is. I'd have to investigate.
- I'm not sure it's useful to generate the asserts: I think the intent was to check 1) the correctness of the manual description, and 2) it being consistent with the eventual tablegen output. Have you considered keeping them in AArch64RegisterBankInfo? I suppose generating them lets all targets have them. @qcolombet: what do you think?
I suppose it depends which direction you look at it. It makes sense to me to have the manual description check that the tablegen part meets its assumptions, but I also think it makes sense to have the tablegen-erated code check that it meets the assumptions of the manual code it's replacing.
For the assertions about PMI_* values, I think the majority of these will become unnecessary once the remaining pieces of AArch64GenRegisterBankInfo.def are tablegen-erated. Once that's done, I think it will all be internal to the tablegen-erated code except for the assumption that PMI_First* can be used to specify the register bank and I think we ought to use *RegBankID's for that instead.
For the assertions that call RB.covers(). I think this can be checked in either direction but it's a bit easier for tablegen to check that all the expected classes have been added since it's easy to forget a particular class when listing them manually.
For the assertions that call RB.getSize(). I think both places are equally good.
- Also, can you extract the RegisterBank code to a shared location? (CodeGenRegBank? maybe rename it to CodeGenRegisters) ? That'll let us reuse it from the selector emitter backend. We can do that separately though.
That makes sense to me. Is it ok if I do that as another patch at the end of my current patch-series? I could insert one before this one too but adding it to the end should save me some rebasing.
utils/TableGen/RegisterBankEmitter.cpp | ||
---|---|---|
343 ↗ | (On Diff #80593) | Ok |
362 ↗ | (On Diff #80593) | Ok |
Yeah, I was worried about that too, but I figured we'll have to generalize that logic at some point anyway, so we might as well clean up the C++ and have a drop-in NFC tablegen replacement.
- I'm not sure it's useful to generate the asserts: I think the intent was to check 1) the correctness of the manual description, and 2) it being consistent with the eventual tablegen output. Have you considered keeping them in AArch64RegisterBankInfo? I suppose generating them lets all targets have them. @qcolombet: what do you think?
I suppose it depends which direction you look at it. It makes sense to me to have the manual description check that the tablegen part meets its assumptions, but I also think it makes sense to have the tablegen-erated code check that it meets the assumptions of the manual code it's replacing.
For the assertions about PMI_* values, I think the majority of these will become unnecessary once the remaining pieces of AArch64GenRegisterBankInfo.def are tablegen-erated. Once that's done, I think it will all be internal to the tablegen-erated code except for the assumption that PMI_First* can be used to specify the register bank and I think we ought to use *RegBankID's for that instead.
For the assertions that call RB.covers(). I think this can be checked in either direction but it's a bit easier for tablegen to check that all the expected classes have been added since it's easy to forget a particular class when listing them manually.
For the assertions that call RB.getSize(). I think both places are equally good.
SGTM, let's reevaluate once .def is gone.
- Also, can you extract the RegisterBank code to a shared location? (CodeGenRegBank? maybe rename it to CodeGenRegisters) ? That'll let us reuse it from the selector emitter backend. We can do that separately though.
That makes sense to me. Is it ok if I do that as another patch at the end of my current patch-series? I could insert one before this one too but adding it to the end should save me some rebasing.
Sure, I'll rebase my patches on top and will copy the regbank code somewhere else for now.
lib/Target/AArch64/AArch64RegisterBankInfo.cpp | ||
---|---|---|
46 ↗ | (On Diff #80593) | How about:
WDYT? |
lib/Target/AArch64/AArch64RegisterBankInfo.cpp | ||
---|---|---|
46 ↗ | (On Diff #80593) | That makes sense to me. |
lib/Target/AArch64/AArch64RegisterBankInfo.cpp | ||
---|---|---|
46 ↗ | (On Diff #80593) |
On thinking about it a bit more, this step doesn't quite work. We can't merge the declaration and implementation together without putting the implementation in the header. I'll keep the declaration of AArch64GenRegisterBankInfo and the implementation separate to avoid putting everything in the header. |
Hi Daniel,
Mostly looks good to me, I would however suggest two changes:
- Drop the partial mapping support.
- Add all the register classes covered by a RegisterBank statically.
For #1, I think that part should belong to something related to instruction pattern emitter.
For #2, the call to addRegBankCoverage still does a bunch of stuff at runtime whereas all doable at compile time. Indeed, I expect the tablegen backend to just populate the bit vector at compile time. Ultimately, I would like to change the representation of the covered register class into an array of uint32_t, so that we just get rid of the complexity of the BitVector. You can have a look at SubClassMask in TargetRegisterInfo and how we generate it to have an idea of what I am talking about.
Cheers,
-Quentin
include/llvm/Target/GlobalISel/RegisterBank.td | ||
---|---|---|
19 ↗ | (On Diff #80593) | This seems gross. The bottom line is, let us clean that up! |
utils/TableGen/RegisterBankEmitter.cpp | ||
38 ↗ | (On Diff #80593) | Partial mappings are more a consequence of instruction patterns than register bank. I wouldn't expect this to happen here. |
202 ↗ | (On Diff #80417) | Right now, this method is used only once. Are there any plan to reuse it with a different functor? |
Thanks. I'm going to put this patch on hold for the moment and post separate patches to lay the necessary groundwork to account for for the changes Ahmed and Quentin requested. I'll then work on updating this patch.
Rewrote patch to work with the new static table based patch series and the related refactoring.
Hi Daniel,
Nitpick stuff, the base is indeed sound and I expect we converge on the final patch quickly.
Thanks,
-Quentin
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
19 ↗ | (On Diff #82088) | I would move that into AArch64RegisterBankInfo.cpp, right before the include of the .def. |
38 ↗ | (On Diff #82088) | Update the "// Idx: " in the comments to match the actual index. |
45 ↗ | (On Diff #82088) | Ditto. |
89 ↗ | (On Diff #82088) | Why do we need three of them instead of none previously? At the very least I would have expected that we need just one of this input. |
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
20 ↗ | (On Diff #82088) | We probably want GET_TARGET_REGBANK_CLASS as well, aren't we? |
26 ↗ | (On Diff #82088) | This shouldn't be "AArch64GenRegisterBankInfo : public RegisterBankInfo" |
lib/Target/AArch64/AArch64RegisterBanks.td | ||
11 ↗ | (On Diff #82088) | I would expect an include of AArch64RegisterInfo.td, otherwise GPR64all and such won't be defined. |
utils/TableGen/RegisterBankEmitter.cpp | ||
32 ↗ | (On Diff #82088) | Which StringRef are you referring to here? |
44 ↗ | (On Diff #82088) | I can see that for debug purposes it is good to get the name of the class with the largest size. Could you update the comment to get rid of the reference to the partial mapping and ExcludePartialMappings stuff, though? |
155 ↗ | (On Diff #82088) | I would add "iff any of this applies:" |
159 ↗ | (On Diff #82088) | You can add (a.k.a. subreg-class) |
162 ↗ | (On Diff #82088) | Add a comment explaining what is RC, Kind, and visitFn. |
172 ↗ | (On Diff #82088) | Period. |
178 ↗ | (On Diff #82088) | Period. |
178 ↗ | (On Diff #82088) | To be a bit more explicit here (or in the comment of the function) we could call out what it means to be a subreg-class: |
220 ↗ | (On Diff #82088) | Before emitting the structure, could we emit a line with a comment with the general structure of each line? |
221 ↗ | (On Diff #82088) | Following the other review, this should change to directly populate the register banks. |
226 ↗ | (On Diff #82088) | Could you add an assert that the current index in the array is equal to the expected value of the enumerator of the Bank? |
232 ↗ | (On Diff #82088) | Following the other review, I would rather have a simple TargetName for the namespace of the register banks. |
235 ↗ | (On Diff #82088) | With the direct initialization of the RegisterBank, we shouldn't need that section anymore, right? |
262 ↗ | (On Diff #82088) | Like I mentioned in my previous review, visitRegisterBankClasses is only used with this visitFn, thus, it seems to me like we could drop this parameter and call the proper function directly in visitRegisterBankClasses. While here, we could rename it addSubClassesAndSubregClasses or something more explicitly like this. |
Rebased onto the new D27339 and re-worked to account for the changes to earlier patches in the series.
Removed the hard-coded 200 register class limit deferred from earlier patches in the series.
Updated to account for most of Quentin's comments. There's a couple I need to ask about.
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
89 ↗ | (On Diff #82088) | We should have had the three of them at the end of the table but got away without them because FPR was last and the missing elements weren't accessed. Now that FPR is first we need them to pad GPR's mappings out to the right indices. I've added comments indicating the invalid mappings they correspond to. |
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
20 ↗ | (On Diff #82088) |
Yes, but it needs to be inside the class declaration for the moment (see below).
When everything is tablegen-erated: Probably not. I don't think there will be a need to keep them separate. For the moment, PartialMappingIdx is the main blocking factor that prevents us from tablegen-erating the whole class and merging these two include points. There are enumerators missing (e.g. FPR192) at the moment. |
26 ↗ | (On Diff #82088) | No, this is the declaration of AArch64GenRegisterBankInfo which will eventually be fully tablegen-erated. AArch64RegisterBankInfo is declared next (line 95 at the moment) as class AArch64RegisterBankInfo final : public AArch64GenRegisterBankInfo. |
lib/Target/AArch64/AArch64RegisterBanks.td | ||
11 ↗ | (On Diff #82088) | This file is included by AArch64.td which includes AArch64RegisterInfo.td on the line before. FWIW, I'm not very keen on all the tablegen files starting from AArch64.td as they currently do. It's wasteful to parse lots of classes and defs that never contribute to the output. We'll need to fix that in tablegen() in our CMake rules before we can start unpicking the dependencies here. |
utils/TableGen/RegisterBankEmitter.cpp | ||
32 ↗ | (On Diff #82088) | I've forgotten to update the comment (and one just below). This used to be a std::vector<std::pair<const CodeGenRegisterClass *, StringRef>> back when the pass emitted assertions for membership. The reason for including a class is now printed in the -debug output. |
220 ↗ | (On Diff #82088) | This table has disappeared in the re-worked patches but I've added similar comments to the generated constructors instead. |
235 ↗ | (On Diff #82088) | That's right. This section has disappeared following that re-work. |
262 ↗ | (On Diff #82088) | The main reason I wrote it this way was to keep the high-level intent (add all the relevant classes) separate from the complexity of how we determine that a given register class is a member. I also needed to factor the current contents of visitFn() into a function (or triplicate it) and I thought it made most sense to put it here rather than in a static function off to the side. I'm happy to fold it into visitRegisterBankClasses() and rename that if that's what you prefer. |
LGTM.
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
20 ↗ | (On Diff #82088) | Ok. |
26 ↗ | (On Diff #82088) | Ah right, I missed the second declaration. |
lib/Target/AArch64/AArch64RegisterBanks.td | ||
11 ↗ | (On Diff #82088) | Good point. Agreed. |
utils/TableGen/RegisterBankEmitter.cpp | ||
262 ↗ | (On Diff #82088) | Up to you. |
LGTM.
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
89 ↗ | (On Diff #82088) | Can't we update the indices or formulae so that we don't need those invalid entries at all? |
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
89 ↗ | (On Diff #82088) | It doesn't look too complicated. We can stop getCopyMapping() needing the invalid entries by dropping the relevant size to index-offset mappings from getRegBankBaseIdxOffset(). However, getValueMapping() also uses that so we'd need to duplicate the function first before trimming the getCopyMapping() version. It would be easiest for me to do this as a follow-up patch but I can insert it into the series if you prefer. |
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
89 ↗ | (On Diff #82088) | Just move forward with the current version. It'll get clean up with the related tablegen support when we add that eventually. |
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
89 ↗ | (On Diff #82088) | Ok, Thanks. |
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
26 ↗ | (On Diff #82088) | So, here's an alternative approach (more in line with what Quentin and I were thinking, I suspect): from day one, generate the entire AArch64GenRegisterBankInfo class. If something currently needs to be manually written code, keep it in AArch64RegisterBankInfo. When tablegen gains support for it, move it from the subclass to the generated base. Because AArch64RegisterBankInfo is-a AArch64GenRegisterBankInfo, all of these changes should be NFC. WDYT? |
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
26 ↗ | (On Diff #82088) | Is there an advantage to re-factoring the patch series that way? It seems equal to the current patch series to me in which case I'm leaning towards the current patches. |
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
26 ↗ | (On Diff #82088) | I suspect it helps make the changes easier to reason about (it's hard to keep track of the moving pieces); other than that, not really, no. Either way is fine by me! |
lib/Target/AArch64/CMakeLists.txt | ||
---|---|---|
3 ↗ | (On Diff #84289) | I didn't notice before, but now that you mention buildbot crashes: this should be in the if(LLVM_BUILD_GLOBAL_ISEL) block below. |
lib/Target/AArch64/CMakeLists.txt | ||
---|---|---|
3 ↗ | (On Diff #84289) | Thanks. It hadn't occurred to me that I had too many buildbots complaining and had started looking for memory bugs. I don't think any GlobalISel bots failed so this is probably it. |