This is an archive of the discontinued LLVM Phabricator instance.

[LAA/LV] Simplify stride speculation logic [nfc]
ClosedPublic

Authored by reames on Apr 6 2023, 5:37 PM.

Details

Summary

The existing code makes it hard to tell that collectStridedAccess is really about identifying some loop invariant SCEV which is *profitable* to speculate is equal to one. The odd dual usage structure of Value and SCEV confuses this point.

We could choose to loosen the profitability analysis if desired. I'm not proposing doing so at this time as it exposes too many cases where the speculation is unprofitable.

Diff Detail

Event Timeline

reames created this revision.Apr 6 2023, 5:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 5:37 PM
reames requested review of this revision.Apr 6 2023, 5:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 5:37 PM
peixin added a subscriber: peixin.EditedApr 7 2023, 1:54 AM

Why do you remove the StrideSet? It can be used to check the const stride in cost model very fast. Now, you check if it is constant one in PSE. The question is if all the const one in PSE are the strides?

Also, it seems that removing the StrideSet will cause the const stride of non-one (such as 4) in D147539 hard to check in cost model.

In addition, I don't figure out how this patch help replace the symbolic stride with constants in vectory body. To perform the replacement, we have the following options:

  1. Emit the constants when generating the vector body. In this way, we need to check every mul operation, which seems to be expansive.
  2. Get all the predicates from PSE and check them, which is done in D147378.
  3. Use the StrideSet in LAA and get the predicate using PSE.

Maybe you have better idea and this patch can help?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6432 ↗(On Diff #511567)
fhahn added inline comments.Apr 7 2023, 5:01 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7208 ↗(On Diff #511567)

This could be done independent of changing the stride map type I think?

fhahn added a comment.Apr 7 2023, 5:09 AM

Why do you remove the StrideSet? It can be used to check the const stride in cost model very fast. Now, you check if it is constant one in PSE. The question is if all the const one in PSE are the strides?

In a way it is slightly more powerful to use PSE directly, as it has info about all predicates (not just about the strides). It should be possible to separate changing the type of the map and replacing stride set. Not sure if there's a big benefit from changing the type of the map; IIUC we will only add SCEVUnknown for the strides anyways.

Also, it seems that removing the StrideSet will cause the const stride of non-one (such as 4) in D147539 hard to check in cost model.

In addition, I don't figure out how this patch help replace the symbolic stride with constants in vectory body. To perform the replacement, we have the following options:

  1. Emit the constants when generating the vector body. In this way, we need to check every mul operation, which seems to be expansive.
  2. Get all the predicates from PSE and check them, which is done in D147378.
  3. Use the StrideSet in LAA and get the predicate using PSE.

Maybe you have better idea and this patch can help?

this is an interesting issue and I experimented with a different approach to solving this. I might be missing something, but it seems like we should be able to directly use the stride map + PSE to add Value->VPValue mappings to the VPlans to get the simplifications on construction: D147783

reames updated this revision to Diff 513257.Apr 13 2023, 8:48 AM
reames retitled this revision from [LAA/LV] Simplify stride speculation logic [nfc-ish] to [LAA/LV] Simplify stride speculation logic [nfc].
reames edited the summary of this revision. (Show Details)

Remove change to StrideSet for now since that triggered a bunch of discussion.

Also, it seems that removing the StrideSet will cause the const stride of non-one (such as 4) in D147539 hard to check in cost model.

This really isn't true JFYI. The whole point of adding a predicate to PSE is that PSE will then *simplify using that predicate*. I'm 100% sure it does the canonicalization today, but it definitely should be and doing so is straight forward.

In a way it is slightly more powerful to use PSE directly, as it has info about all predicates (not just about the strides). It should be possible to separate changing the type of the map and replacing stride set.

I separated the changes since it triggered more discussion than I'd really expected.

Not sure if there's a big benefit from changing the type of the map; IIUC we will only add SCEVUnknown for the strides anyways.

We return only SCEVUnknown today. That's an implementation detail of the *profitability* heuristic in this code. We *could* return any loop invariant SCEV which is useful to speculate on. I added a clarifying comment and restructured the patch description to emphasize this.

fhahn added a reviewer: Ayal.May 8 2023, 10:03 AM

LGTM as this simplifies things slightly by avoiding a Value <-> SCEV roundtrip. Please wait a day or so with committing in case other reviewers have additional thoughts.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
3480

nit: could just be auto &, given hat the type is already clear from the object construction.

Ayal added a comment.May 8 2023, 4:49 PM

LGTM, worth using ValueToSCEVMap = DenseMap<const Value *, const SCEV *> ?

llvm/include/llvm/Analysis/LoopAccessAnalysis.h
703

Documentation here and above still seems fine, right?

720–721

80 columns?

llvm/lib/Analysis/LoopAccessAnalysis.cpp
2693

LGTM, worth using ValueToSCEVMap = DenseMap<const Value *, const SCEV *> ?

I looked at this, but I don't think it helps clarify. We have an existing ValueToSCEVMapTy which differs in constness of the key. We've also got both ValueToValueMap and ValueToValueMapTy (they're different). As a cleanup after this, I'm going to go take a look and see if I can clean these up slightly, but I'm not expecting huge clarity wins in this code.

This revision was not accepted when it landed; it landed in state Needs Review.May 11 2023, 8:33 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.