This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Lower G_BUILD_VECTOR -> G_DUP
ClosedPublic

Authored by paquette on Mar 1 2021, 1:51 PM.

Details

Summary

If we have

%vec = G_BUILD_VECTOR %reg, %reg, ..., %reg

Then lower it to

%vec = G_DUP %reg

Also update the selector to handle constant splats on G_DUP.

Minor code size improvements on CTMark at -Os.

Diff Detail

Event Timeline

paquette created this revision.Mar 1 2021, 1:51 PM
paquette requested review of this revision.Mar 1 2021, 1:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 1:51 PM
paquette updated this revision to Diff 327636.Mar 2 2021, 5:42 PM
paquette retitled this revision from [AArch64][GlobalISel] Lower G_BUILD_VECTOR -> G_DUP for non-constant splats to [AArch64][GlobalISel] Lower G_BUILD_VECTOR -> G_DUP.
paquette edited the summary of this revision. (Show Details)
aemerson added inline comments.Mar 2 2021, 9:50 PM
llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerLowering.cpp
475

I think we should this be a helper class and live in some AArch64 specific utils somewhere. We should programmatically have a single query to test for a splat operation.

s/class/function.

paquette updated this revision to Diff 327674.Mar 2 2021, 10:31 PM
  • Fix a bug in G_DUP for preISelLower and add a testcase which exercises it to select-dup.mir
  • Add getAArch64VectorConstantSplat, which checks for G_DUP as well as G_BUILD_VECTOR. Use that instead in places which now need to check for G_DUP.

(There's one place where the helper might make some difference, since getBuildVectorConstantSplat doesn't look through constants.)

Looks like this blocks selecting vector neg since we miss some imported patterns. E.g.

// (sub:{ *:[v2i32] } immAllZerosV:{ *:[v2i32] }, V64:{ *:[v2i32] }:$Rn)  =>  (NEGv2i32:{ *:[v2i32] } V64:{ *:[v2i32] }:$Rn)

Is there any way to get a G_DUP with 0 to match here?

Looks like this blocks selecting vector neg since we miss some imported patterns. E.g.

// (sub:{ *:[v2i32] } immAllZerosV:{ *:[v2i32] }, V64:{ *:[v2i32] }:$Rn)  =>  (NEGv2i32:{ *:[v2i32] } V64:{ *:[v2i32] }:$Rn)

Is there any way to get a G_DUP with 0 to match here?

:(

I don't see any easy way. You could maybe replicate the patterns that use immAllZeroesV to have a variant that uses AArch64dup but that doesn't seem like the right approach. I guess we have to avoid using this for constants in the absence of a better idea.

paquette updated this revision to Diff 328328.Mar 4 2021, 3:49 PM

This time, disallow 0 and 1 specifically.

I think there are some DUP combines we're going to want. If 90% of the time it's better to represent this as DUP, I think it's probably better to prefer that.

Also improve the helpers a little bit. I figured it'd be nice to have a general helper which gives you either a constant or a register for the dup matching.

aemerson added inline comments.Mar 4 2021, 11:53 PM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
263

A quick comment to explain what this is for would be helpful. I also think we have the space to fully spell out Constant in the type.

267

This is implementing a tagged union, and since it's better to not try to cast a Register to an int64 here, try:

class RegOrConstant {
  union {
    int64_t IntVal;
    Register Reg;
  };
  bool IsReg;
public:
  explicit RegOrConstant(int64_t Cst) : IntVal(Cst), IsReg(false) {}
  explicit RegOrConstant(Register R) : Reg(R), IsReg(true) {}
  Register getReg()...
... etc
};
llvm/lib/Target/AArch64/GISel/AArch64GlobalISelUtils.h
23

The types used in the declarations aren't opaque pointers, so forward declaring doesn't help here.

31

These helpers should be names could be more precise, because they seem to return a scalar, whereas "splat" usually refers to the vector itself. Something like getAArch64VectorSplatSrc() or getAArch64VectorSplatScalar() might be better.

llvm/lib/Target/AArch64/GISel/AArch64PostLegalizerLowering.cpp
707

I'd specifically call out the immAllZeroesV and immAllOnesV patterns in this comment.

paquette added inline comments.Mar 5 2021, 5:13 PM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
263

Like rename it to Constant?

267

I actually wrote this as a union originally, but it would do the wrong thing if Register has a non-trivial destructor or non-trivial copy constructor, so I just avoided the whole issue.

aemerson added inline comments.Mar 6 2021, 9:09 PM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
263

Yeah, RegOrConstant.

267

Ah yeah. In that case, I think it's still better to use extra memory and store both a Register and an in64_t, rather than casting.

paquette updated this revision to Diff 329059.Mar 8 2021, 10:15 AM
paquette marked 4 inline comments as done.

Address review comments.

aemerson accepted this revision.Mar 8 2021, 11:08 AM

LGTM.

This revision is now accepted and ready to land.Mar 8 2021, 11:08 AM
This revision was landed with ongoing or failed builds.Mar 8 2021, 1:02 PM
This revision was automatically updated to reflect the committed changes.