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
4429

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
692

Do you need $info here? It looks unused.

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

Do you mean D110747?

4536

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

4561–4562

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?

4585

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
4612

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
4429

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
692

Nope, it's unused.

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

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

4536

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

4561–4562

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
603

could use a doxygen comment?

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

doxygen comment should be in the header, right?

4475

might as well have a message here

4485

might as well have a message here too

4512

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

4535

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
4419

What's this for?

4561–4562

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
4419

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.