This is an archive of the discontinued LLVM Phabricator instance.

[riscv] Prefer to use previous VL for scalar move instructions
ClosedPublic

Authored by reames on May 10 2022, 1:36 PM.

Details

Summary

This patch is an alternative to a piece of D125270. It's direct motivation is to fix a wrong code bug (described below), but somewhat unexpectedly, it also results in a significant code quality improvement for idiomatic fixed length vector patterns.

The existing transform is simply wrong in its current location. We are correct about the fact that the scalar move itself can use the previous vsetvli, but we loose track of the fact that later instructions might depend on the state change represented. That is, the actual value of VL in the register is different than the abstract state thinks it is. Not simplify due to precision of modeling, but e.g. the VL register could contain 3 when the abstract state says it is 1. This is annoying hard to demonstrate in practice due to differences in policy flags on the intrinsics, but this is at least a latent wrong code bug.

The code quality benefit comes from the fact we don't need to tie this to explicit vsetvli instructions at all. We can propagate the abstract state, and reduce a) the number of transitions, or b) the cost of those transitions. It turns out we have a bunch of cases - in tests at least - where fixed length AVLs are known non-zero, and we can leave VL unchanged while changing VTYPE.

Diff Detail

Event Timeline

reames created this revision.May 10 2022, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 1:36 PM
reames requested review of this revision.May 10 2022, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 1:36 PM
reames updated this revision to Diff 428515.May 10 2022, 3:25 PM
craig.topper accepted this revision.May 10 2022, 4:49 PM

I think the "Not simplify due to precision" was supposed to say "simply"?

LGTM. I like this a lot better.

Can this replace the scalar move handling that's still in VSETVLIInfo::isCompatible?

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1229

"there is" -> "there are"

1231

"remove" -> "removes"

1255

"something updates" -> "something that updates". I think that's a mistake where you copied this from too.

This revision is now accepted and ready to land.May 10 2022, 4:49 PM
frasercrmck accepted this revision.May 11 2022, 1:45 AM

Also LGTM aside from nits and possible restructuring. Definitely nicer than the other patch.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1231

setvli -> vsetvli?

1232

or if it allows the use of a cheaper

1234

vsetvli again? maybe a comma after that last x0 too

1234

and avoid -> so (we) avoid?

1251

If we don't update the scalar move instruction this is falling through and computing the same as NewInfo. I dunno if you think it'd be clearer if we were explicit about that in the previous block (e.g. update CurInfo and continue in an else?)

You could even early-continue if MI has a SEW op and it isn't a scalar move.

1258

Do we need parens on this if?

I think the "Not simplify due to precision" was supposed to say "simply"?

Yes.

Can this replace the scalar move handling that's still in VSETVLIInfo::isCompatible?

Not sure, but I suspect not. Will look more closely in follow up.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1232

The wording is intentional. As written, it is explicitly an xor for the condition under which we run. Yours leaves open the possibility of an else.

1251

In the current patch, all three are reasonable. I think the current form is fairly clear, and it simplifies a patch I'm about to post. :)

This revision was landed with ongoing or failed builds.May 11 2022, 7:41 AM
This revision was automatically updated to reflect the committed changes.