Page MenuHomePhabricator

[AArch64][SVE][VLS] Move extends into arguments of comparisons

Authored by DavidTruby on Jan 7 2022, 7:56 AM.



When a comparison is extended and it would be free to extend the
arguments to that comparison, we can propagate the extend into those arguments.
This prevents extra instructions being generated to extend the result of the
comparison, which is not free to extend.

Diff Detail

Event Timeline

DavidTruby created this revision.Jan 7 2022, 7:56 AM
DavidTruby requested review of this revision.Jan 7 2022, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2022, 7:56 AM

Fix failing tests

Looks reasonable to me. A suggestion inline.

One other request, please could we have a brief comment or lispy expression on the function which does the combine to indicate why it exists / what the intent is.


I see you've protected N->getOperand(0) with this assert, but what about N->getOpcode()?

I would like to see a structure that looks a bit more like performORCombine, for example which looks like the below. That would pull the conditions into this function and make this assert into a if (!...) return SDValue() instead.

The motivation behind my ask is to put all the logic for this combine in this function.

static SDValue performORCombine(SDNode *N, TargetLowering::DAGCombinerInfo &DCI,
                                const AArch64Subtarget *Subtarget) {
  if (SDValue Res = tryCombineToEXTR(N, DCI))
    return Res;

static SDValue tryCombineToEXTR(SDNode *N,
                                TargetLowering::DAGCombinerInfo &DCI) {
  assert(N->getOpcode() == ISD::OR && "Unexpected root");
  // ...
Matt added a subscriber: Matt.Jan 13 2022, 10:48 AM

Expand assert to protect N->getOpcode()

Add comment to combine to explain its purpose

DavidTruby added inline comments.Jan 17 2022, 10:19 AM

I wrote it this way following some of the other functions in this file, it doesn't seem there's much consistency between this way and they way you're suggesting!

My gut instinct tells me it's clearer in this case to have the condition in the other function and the assertion here, because then it shows clearly in which circumstances you're going to call this combine. I'm happy to change it if you think it would be better the other way around though!

peterwaller-arm accepted this revision.Jan 18 2022, 2:12 AM
peterwaller-arm added inline comments.

I see tests hitting the MLOAD case, but not the LOAD nor isConstantSplatVectorAllZeros cases. Any thoughts on that?

Another idea that pops into mind: I might expect that all constants could be cheap to extend, as logically they can be rewritten as another constant. I see that FoldConstantArithmetic might achieve this in getNode for the SIGN_EXTEND. Not necessarily for this patch unless you agree and think it's easy to do.


I agree there are different ways this is written, the above comment is only a suggestion so I'm happy to proceed as is.

This revision is now accepted and ready to land.Jan 18 2022, 2:12 AM

I see tests hitting the MLOAD case, but not the LOAD nor isConstantSplatVectorAllZeros cases. Any thoughts on that?

Thanks for pointing out to me offline that the test cases do hit these (of course!).

This revision was landed with ongoing or failed builds.Jan 19 2022, 6:12 AM
This revision was automatically updated to reflect the committed changes.

Hi @DavidTruby, this patch causes a build failure on MultiSource/Applications/JM/lencod (file rc_quadratic.c) when enabling SVE. Can you please investigate and revert/fix?

paulwalker-arm added inline comments.Jan 20 2022, 3:49 AM

This doesn't look correct. The extension type should mirror the comparison operator. So whilst signed comparisons require the original operands to be sign-extended, unsigned comparisons will require the operands to be zero extended to match the original behaviour.