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
4430

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
4430

Do you mean D110747?

4537

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

4562–4563

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?

4586

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
4613

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
4430

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
4430

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

4537

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

4562–4563

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
4409

doxygen comment should be in the header, right?

4476

might as well have a message here

4486

might as well have a message here too

4513

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

4536

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
4420

What's this for?

4562–4563

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
4420

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.