This is an archive of the discontinued LLVM Phabricator instance.

[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

anwel created this revision.Mar 25 2020, 10:43 AM
samparker added inline comments.Mar 27 2020, 3:17 AM
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
787

You could use getIncomingValueForBlock instead.

804

Would be nicer to cast to a GEP just once.

857

Looking at the manual, it looks like it's a signed immediate and are you checking that it's a multiple of 4?

864

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
306

Will this ever be false? If so, the place this is called might be casting a nullptr.

770

I think LI should already be available. It's common to store analyses like that as a member of the pass.

785

This can be merged into the previous if?

797

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)

800

aligned -> scaled

911–916

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
306

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.

770

Alright, I made it a class member, so it'll only be queried once.

785

Yes, seems like it can.

797

You're right. I did change it to dyn_cast and merge the ifs though.

804

That's true. Done.

857

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.

864

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.

911–916

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
796

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?

831

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?

850

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.

875

aligned -> scaled

985

Is Ignore ever used in this code? If not, would it be simpler to just remove it?

992–993

Do these extends/truncates ever come up in the tests here?

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
796

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.

831

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.

985

No, the parameter is indeed not needed anymore.

985

Ignore has indeed become superfluent.

992–993

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.

992–993

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.

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
359 ↗(On Diff #258728)

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
231–1

Give this a clang-format?

232

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

232

I think this might as well always set LI.

234

Can GEP ever be null here?

236

You don't need else's after returns.

254

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

273

Can be removed.

294–296

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

309

= 1 - IncrementIndex;

319

This is overwritten below I think

350

It looks like Phi is always valid here.

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
359 ↗(On Diff #258728)

Alright, I moved it there

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

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

254

Okay.

294–296

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

319

Yes, you are right.

350

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.May 4 2020, 4:34 AM

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

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

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

287

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

292

Creating a variable for L sounds useful.

301

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

328

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.May 4 2020, 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
287

Has been added.

301

Yes, you are right.

328

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.May 5 2020, 8:38 AM
llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp
271

I feel like this should be using BasePtr somewhere?

278

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.May 6 2020, 6:18 AM
anwel marked 2 inline comments as done.

Applied the chances suggested in the comments.

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

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

278

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.May 6 2020, 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
290

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.May 6 2020, 11:39 PM
anwel updated this revision to Diff 262593.May 7 2020, 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.May 7 2020, 3:51 AM
This revision was automatically updated to reflect the committed changes.