Page MenuHomePhabricator

[ARM][MVE] Add support for incrementing gathers
ClosedPublic

Authored by anwel on Mar 25 2020, 10:43 AM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
samparker added inline comments.Mar 27 2020, 3:17 AM
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?

anwel updated this revision to Diff 253547.Mar 30 2020, 4:16 AM
anwel marked 17 inline comments as done.

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!
The value that is in the end handed over to the gather as increment will be a multiple of four, as we are shifting it left to replicate the change it would have experienced from the GEP we replaced.

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.

dmgreen added inline comments.Apr 1 2020, 4:29 AM
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

anwel updated this revision to Diff 257651.Apr 15 2020, 3:23 AM
anwel marked 8 inline comments as done.

Made some changes suggested in the comments, and also re-ordered the code such that it is easier to follow.

anwel added inline comments.Apr 16 2020, 3:25 AM
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.
I have decided to re-work this function from generally checking whether a value is a loop invariant or constant to checking whether it is a constant or arithmetic expression a constant can be derived from, and made it so that it returns that constant. This reduces code duplication at the location the function is called, and also makes it more clear what is happening.

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.
I have decided to re-work this function from generally checking whether a value is a loop invariant or constant to checking whether it is a constant or arithmetic expression a constant can be derived from, and made it so that it returns that constant. This reduces code duplication at the location the function is called, and also makes it more clear what is happening.

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.

anwel updated this revision to Diff 258723.Apr 20 2020, 7:02 AM

Fixed calculation of the constant in getIfConst, moved isGatherScatter to ARMBaseInstrInfo so it is accessible for other passes, too.

anwel marked 5 inline comments as done.Apr 20 2020, 7:03 AM
anwel updated this revision to Diff 258728.Apr 20 2020, 7:08 AM
dmgreen added inline comments.Apr 22 2020, 7:47 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
360

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
165–166

Give this a clang-format?

245

I think you may be able to drop all the llvm::'s from the Optional's

249

You don't need else's after returns.

429

Can GEP ever be null here?

478

This should probably be using LI->getLoopLatch(), in case there are multiple blocks in the loop.

497

Can be removed.

518–520

Can this be OffsetsIncoming != Phi? Or am I missing what this is checking?

533

= 1 - IncrementIndex;

543

This is overwritten below I think

574

It looks like Phi is always valid here.

941

I think this might as well always set LI.

anwel updated this revision to Diff 260594.Apr 28 2020, 4:51 AM
anwel marked 18 inline comments as done.
anwel added inline comments.Apr 28 2020, 4:52 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.h
360

Alright, I moved it there

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
429

Now that you mention it, no it can't.

478

Okay.

518–520

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

543

Yes, you are right.

574

It is in fact.

anwel updated this revision to Diff 260874.Apr 29 2020, 3:13 AM

Fixing a bug in the latest diff: We should always make sure to scale the offsets that the loop is initialised with.

anwel updated this revision to Diff 261781.Mon, May 4, 4:34 AM

Restructured the code such that the differentiation between writeback and non-writeback gathers is clearer

dmgreen added inline comments.Mon, May 4, 5:41 AM
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
42

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).

511

Maybe add a check that Phi->getBB == L->getHeader too.

516

Creating a variable for L sounds useful.

525

I think we have to check that OffsetsIncoming == Phi. To make sure it's the loop structure / IV we want it to be.

552

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.

anwel updated this revision to Diff 261818.Mon, May 4, 7:47 AM
anwel marked 8 inline comments as done.

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
511

Has been added.

525

Yes, you are right.

552

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.

dmgreen added inline comments.Tue, May 5, 8:38 AM
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
495

I feel like this should be using BasePtr somewhere?

502

This looks OK. Can you add one extra check somewhere that gep has a single user? As the phi is changing and the gep uses the phi.

anwel updated this revision to Diff 262355.Wed, May 6, 6:18 AM
anwel marked 2 inline comments as done.

Applied the chances suggested in the comments.

anwel marked 2 inline comments as done.Wed, May 6, 6:21 AM
anwel added inline comments.
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
495

You are right, the base pointer should be added to the offsets before handing them over to the intrinsic construction. I fixed that now.

502

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.

dmgreen accepted this revision.Wed, May 6, 11:39 PM

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
514

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.

This revision is now accepted and ready to land.Wed, May 6, 11:39 PM
anwel updated this revision to Diff 262593.Thu, May 7, 3:50 AM
anwel marked an inline comment as done.

Last correction: For the incrementing non-writeback gathers, removed the check that the offset only has one use.

anwel marked an inline comment as done.Thu, May 7, 3:51 AM
This revision was automatically updated to reflect the committed changes.