This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering][RISCV][AArch64][PowerPC] Enable BuildUDIV/BuildSDIV on illegal types before type legalization if we can find a larger legal type that supports MUL.
ClosedPublic

Authored by craig.topper on Feb 6 2021, 11:33 AM.

Details

Summary

If we wait until the type is legalized, we'll lose information
about the orginal type and need to use larger magic constants.
This gets especially bad on RISCV64 where i64 is the only legal
type.

I've limited this to simple scalar types so it only works for
i8/i16/i32 which are most likely to occur. For more odd types
we might want to do a small promotion to a type where MULH is legal
instead.

Unfortunately, this does prevent some urem/srem+seteq matching since
that still require legal types.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 6 2021, 11:33 AM
craig.topper requested review of this revision.Feb 6 2021, 11:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2021, 11:33 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
RKSimon added inline comments.Feb 9 2021, 8:33 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5101

Do we not have a legalization helper function that we can use?

5183

Pre-legalization shift amount is always nasty - is there any chance that this can be out of range?

llvm/test/CodeGen/AArch64/urem-seteq.ll
86

No idea why but its odd that we now match a multiply-subtract here but lose the multiply-add in urem-seteq-nonzero.ll above

craig.topper added inline comments.Feb 9 2021, 9:23 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5101

I thought about using getTypeAction() == TypePromoteInteger and getTypeToTransformTo. And now I can't remember why I didn't.

5183

MulVT here is a legal type so the shift should be legal. So I think the target better be returning a shift amount type that should cover all possible amounts.

llvm/test/CodeGen/AArch64/urem-seteq.ll
86

The msub here is part of the remainder calculation. The case above used a divisor of 3 which is one more than a power of 2 so the multiply for the remainder was turned into "add w8, w8, w8, lsl #1"

We're using different algorithms now so its hard to say much more than that.

RKSimon added inline comments.Feb 9 2021, 9:57 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5183

Would getShiftAmountConstant be safer?

craig.topper added inline comments.Feb 9 2021, 10:08 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5183

It's shorter. I don't think its safer though. It just chooses between getScalarShiftAmountTy and PointerTy. It doesn't check that the constant fits.

RKSimon added inline comments.Feb 9 2021, 12:14 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5183

OK its still probably worth trying to use getShiftAmountConstant.

Use getShiftAmountConstant

Cheers Craig - any thoughts on how best to tidy up the legal type finder code?

Use getTypeAction/getTypeToTransformTo to remove the loop.

RKSimon accepted this revision.Feb 11 2021, 6:32 AM

LGTM

This revision is now accepted and ready to land.Feb 11 2021, 6:32 AM
This revision was landed with ongoing or failed builds.Feb 11 2021, 9:44 AM
This revision was automatically updated to reflect the committed changes.