This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Look through truncs and extends in narrowScalarShift
ClosedPublic

Authored by kschwarz on Oct 9 2020, 1:02 AM.

Details

Summary

If a G_SHL is fed by a G_CONSTANT, the lower and upper bits of the source can be
shifted individually by the constant shift amount.

However in case the shift amount came from a G_TRUNC(G_CONSTANT), the generic shift legalization
code was used, producing intermediate shifts that are potentially illegal on some targets.

This change teaches narrowScalarShift to look through G_TRUNCs and G_*EXTs.

Diff Detail

Event Timeline

kschwarz created this revision.Oct 9 2020, 1:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2020, 1:02 AM
kschwarz requested review of this revision.Oct 9 2020, 1:02 AM

The code asserting in GISelKnownBits is probably broken. An out of bounds shift is not illegal IR

The code asserting in GISelKnownBits is probably broken. An out of bounds shift is not illegal IR

I've created https://reviews.llvm.org/D89232 to fix the code in GISelKnownBits.cpp

I still think the optimization done in this patch is valuable though (with an updated commit message, of course). What do you think?

arsenm added inline comments.Oct 12 2020, 10:45 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4190

Why do you need to get DefMI? VRegAndVal should have the value already? (Or is this because it stores an int64_t? I've been thinking we should maybe start returning the ConstantInt or APInt instead)

kschwarz added inline comments.Oct 12 2020, 11:54 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4190

Yes, the motivation was to stick with the existing API and not having to touch narrowScalarShiftByConstant.
Returning APInt from getConstantVRegValWithLookThrough would make this code simpler, but I don't know if its generally preferable? In any case, that would be a separate change, right?

kschwarz updated this revision to Diff 316903.Jan 15 2021, 4:30 AM

Adapt to new API of getConstantVRegValWithLookThrough, rebase

kschwarz updated this revision to Diff 365099.Aug 9 2021, 12:44 AM
kschwarz edited the summary of this revision. (Show Details)

Rebase + test updates

paquette accepted this revision.Aug 9 2021, 9:56 AM

I think this seems reasonable with a minor nit

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4192

Reuse ShiftAmtTy?

This revision is now accepted and ready to land.Aug 9 2021, 9:56 AM
kschwarz updated this revision to Diff 365430.Aug 10 2021, 4:45 AM

Use available ShiftAmtTy

This revision was landed with ongoing or failed builds.Aug 10 2021, 4:50 AM
This revision was automatically updated to reflect the committed changes.