Depends on D27809
Details
Diff Detail
- Build Status
Buildable 2778 Build 2778: arc lint + arc unit
Event Timeline
Hi Daniel,
LGTM modulo a couple of nitpicks.
Cheers,
-Quentin
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
150 | 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. | |
78 | const std::vector<...> & |
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. |
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
150 | 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 |
Ok.
Well spotted |
lib/Target/AArch64/AArch64GenRegisterBankInfo.def | ||
---|---|---|
150 |
That would be my preference.
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. |
Rebased onto new D27809.
Also, the *RegBank globals are back in the llvm::<Target> namespace and
assertPartialMappingIdx() is now checkPartialMappingIdx() as requested.
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? |
This LGTM, but I'll let Quentin have final say.
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
32 |
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> ? |
lib/Target/AArch64/AArch64RegisterBankInfo.h | ||
---|---|---|
78 | That worked nicely. Thanks |
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.