This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Port the udiv -> mul by constant combine.
ClosedPublic

Authored by aemerson on Sep 30 2021, 4:02 PM.

Details

Summary

This is a straight port from the equivalent DAG combine.

For AMDGPU, since it doesn't support legalizing G_MULH for s64 element vectors I've added an abort to the run line to get it to pass.

Diff Detail

Event Timeline

aemerson created this revision.Sep 30 2021, 4:02 PM
aemerson requested review of this revision.Sep 30 2021, 4:02 PM

@arsenm @foad The udiv.i64.ll test has had an abort line put it because this patch causes a G_MULH of <2 x s64> to be generated which is currently not legalized for AMDGPU.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4447

This function currently copied but can be shared with the DAG, but I'm waiting for D98200 to land so we can put it in a common utils file.

foad added inline comments.Oct 1 2021, 3:29 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
699

Do you need $info here? It looks unused.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4447

Do you mean D110747?

4554

Can this ever happen? If not, just assert it returns true? Is that what the comment in applyUDivByConst is referring to?

4579–4580

Does the umulh really work out cheaper than (say) shifting NPQ right by one, and then ANDing that with a mask of -1s and 0s?

4603

Fold this into the "if".

foad added a comment.Oct 1 2021, 5:36 AM

@arsenm @foad The udiv.i64.ll test has had an abort line put it because this patch causes a G_MULH of <2 x s64> to be generated which is currently not legalized for AMDGPU.

D110926 should fix it.

paquette added inline comments.Oct 1 2021, 11:39 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4630

can probably save a line of code here

if (auto *CI = dyn_cast_or_null...)
   return !CI->isZero();
return false;
llvm/lib/CodeGen/GlobalISel/Utils.cpp
1030

can this just be

if (getIConstantVRegValWithLookThrough... || getOpcodeDef<GImplicitDef>...)
  continue;
ctetreau added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4447

This patch is merged now, by the way. I'm glad there's another usage upstream now, so hopefully nobody tries to make it a static local again.

aemerson added inline comments.Oct 1 2021, 3:19 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
699

Nope, it's unused.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4447

I wasn't aware of that patch actually, but yes I'll use that instead.

4554

Yes this should just be an assert. The comment was before I refactored some of this.

4579–4580

I think we can implement another combine to optimize the G_MULHU like the DAG does in this case?

aemerson updated this revision to Diff 376668.Oct 1 2021, 9:05 PM

Address most comments.

paquette added inline comments.Oct 4 2021, 9:56 AM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
606

could use a doxygen comment?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4426

doxygen comment should be in the header, right?

4493

might as well have a message here

4503

might as well have a message here too

4530

Should there be a legalizer check here? I think this will only work pre-legalize.

4553

comment explaining why we only do this for size?

aemerson updated this revision to Diff 376981.Oct 4 2021, 12:06 PM
foad added a comment.Oct 5 2021, 5:36 AM

@arsenm @foad The udiv.i64.ll test has had an abort line put it because this patch causes a G_MULH of <2 x s64> to be generated which is currently not legalized for AMDGPU.

D110926 should fix it.

That didn't work, but D111129 should fix it instead.

paquette accepted this revision.Oct 5 2021, 4:59 PM

Personally I think this is probably good to go?

@foad, does this look good on the AMDGPU end?

This revision is now accepted and ready to land.Oct 5 2021, 4:59 PM
foad added a comment.Oct 6 2021, 1:20 AM

Can you rebase on D111129 and remove the abort you added?

foad added inline comments.Oct 6 2021, 1:37 AM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
400

Seems odd to mention "splat" at all here.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4437

What's this for?

4579–4580

Well D111036 as it stands does not go far enough. It does not handle non-splat constant vectors, and it does not handle zeros in vectors.

I still think using G_UMULH and relying on it being cleaned up later is a strange implementation choice, but I guess if D111036 can be improved then it's OK.

aemerson added inline comments.Oct 7 2021, 10:35 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4437

This is just to shorten the code later.

This revision was landed with ongoing or failed builds.Oct 7 2021, 11:37 AM
This revision was automatically updated to reflect the committed changes.