Enables the MVEGatherScatterLowering pass to build pre-incrementing gathers.
If the increment (i.e., add instruction) that is merged into the gather is the loop increment, an incrementing write-back gather can be built, which then assumes the role of the loop increment.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
958 | You could use getIncomingValueForBlock instead. | |
975 | Would be nicer to cast to a GEP just once. | |
1028 | Looking at the manual, it looks like it's a signed immediate and are you checking that it's a multiple of 4? | |
1035 | Could these checks be together with the other phi stuff around line 788? |
Tests look nice on this one.
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
371 | Will this ever be false? If so, the place this is called might be casting a nullptr. | |
941 | I think LI should already be available. It's common to store analyses like that as a member of the pass. | |
956 | This can be merged into the previous if? | |
968 | I don't think Add will be nullptr here. (cast cannot fail by returning nullptr. dyn_cast would, but Offset is already known to be an Instruction) | |
971 | aligned -> scaled | |
1082–1087 | Can you move the if into the argument? Also is it worth having tryCreateMaskedGatherBaseWB just return an IntrinsicInst? |
Changes addressing most of the comments - see below.
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
371 | With the current usage, this should never be false - tryCreateMergedGatherIncrement currently is the only user and will have checked this condition long before the gather is built. I'd still like to leave this check in here for the future, as we might add another use of this function where it might not be clear. | |
941 | Alright, I made it a class member, so it'll only be queried once. | |
956 | Yes, seems like it can. | |
968 | You're right. I did change it to dyn_cast and merge the ifs though. | |
975 | That's true. Done. | |
1028 | You are right, we should be checking the lower bound, too - thanks for catching that! | |
1035 | These checks are to determine whether a non-incrementing gather should be built and immediately call that function - which requires the variable Const (calculated above) to be handed over. So I couldn't move this up to line 788, at least not as a block. | |
1082–1087 | Yes, can do. I'd rather have all the functions creating gathers or scatters having the same return type. It is however no longer necessary to have an IntrinsicInst here, so I just removed the cast. |
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
967 | How come this is testing multiple users? Don't we have to test the one that related to our gather? Otherwise if there were multiple and they had different scales, might we pick the wrong one? | |
990 | Is Ignore ever used in this code? If not, would it be simpler to just remove it? | |
997–998 | Do these extends/truncates ever come up in the tests here? | |
1002 | uint64_t is better for constants values. Also if you are using them signed, then they should maybe be getSExtValue, and use int64_t instead? | |
1021 | I think it might be something like this maybe? int64_t Immediate = Const << TypeScale; if (Immediate > 512 || Immediate < -512 || Immediate % 4 != 0) As in it is +/- 7bit imm * 4 in the instruction. | |
1046 | aligned -> scaled |
Made some changes suggested in the comments, and also re-ordered the code such that it is easier to follow.
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
967 | You are right. We need information about the specific GEP used by the gather here. I have made some changes to make sure that information is present. | |
990 | No, the parameter is indeed not needed anymore. | |
990 | Ignore has indeed become superfluent. | |
997–998 | No, they don't. If they ever come up in the context in which this function is used, we will later decide to abort anyway. | |
997–998 | No, they don't. If they ever come up in the context in which this function is used, we will later decide to abort anyway. | |
1002 | That is a good point, the constants we're looking at here could be negative so signed extension will be the right way to go. I'll make it an int64_t then. |
Fixed calculation of the constant in getIfConst, moved isGatherScatter to ARMBaseInstrInfo so it is accessible for other passes, too.
llvm/lib/Target/ARM/ARMBaseInstrInfo.h | ||
---|---|---|
25 | This needn't be in the ARMBaseInstrInfo class specifically. There are some free functions at the end of this file. I think it can go with those. | |
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
226–1 | Give this a clang-format? | |
227 | I think you may be able to drop all the llvm::'s from the Optional's | |
227 | I think this might as well always set LI. | |
229 | Can GEP ever be null here? | |
231 | You don't need else's after returns. | |
249 | This should probably be using LI->getLoopLatch(), in case there are multiple blocks in the loop. | |
268 | Can be removed. | |
289–291 | Can this be OffsetsIncoming != Phi? Or am I missing what this is checking? | |
304 | = 1 - IncrementIndex; | |
314 | This is overwritten below I think | |
345 | It looks like Phi is always valid here. |
llvm/lib/Target/ARM/ARMBaseInstrInfo.h | ||
---|---|---|
25 | Alright, I moved it there | |
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
229 | Now that you mention it, no it can't. | |
249 | Okay. | |
289–291 | Not exactly, because Phi can be nullptr in some cases where OffsetsIncoming still is a phi node. Using the latter instead of checking all operands of Add again does make this code more readable, though | |
314 | Yes, you are right. | |
345 | It is in fact. |
Fixing a bug in the latest diff: We should always make sure to scale the offsets that the loop is initialised with.
Restructured the code such that the differentiation between writeback and non-writeback gathers is clearer
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
227 | You don't need experimental/optional (we technically need llvm/ADT/Optional.h I think, as we are using the one in llvm. It is likely included transitively at the moment). | |
282 | Maybe add a check that Phi->getBB == L->getHeader too. | |
287 | Creating a variable for L sounds useful. | |
296 | I think we have to check that OffsetsIncoming == Phi. To make sure it's the loop structure / IV we want it to be. | |
323 | As we are modifying the Phi inplace, and it can produce different values, I think we unfortunately need to check it has no other uses. |
Made optimisation of offsets independent of the creation of the individual gathers and scatters, such that when looking at transforming those we're not in an environment where some instructions have been optimised but others, which will be optimised too, haven't been yet.
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
282 | Has been added. | |
296 | Yes, you are right. | |
323 | You convinced me, though it is sad we have to restrict the usage of the writeback gathers that much. I'll leave it to future work to make it right and find a way to allow them being used more often again. |
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
266 | You are right, the base pointer should be added to the offsets before handing them over to the intrinsic construction. I fixed that now. | |
273 | You are right, unfortunately we cannot allow this as is with gep having more than one user. This means for now that in cases where multiple gathers and/or scatters use the same gep we cannot make any of them write the increment back, which we might want to fix in a future patch. |
Thanks. I think in hindsight this might have been a couple of patches, we we managed to power through.
LGTM with one minor alteration
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp | ||
---|---|---|
285 | I think that in this case we needn't check for one use of Offsets. Because we are only folding it into the instruction, not altering it in any way, it should always be OK I think. And smaller. But, er, check that makes sense. |
Last correction: For the incrementing non-writeback gathers, removed the check that the offset only has one use.
This needn't be in the ARMBaseInstrInfo class specifically. There are some free functions at the end of this file. I think it can go with those.