This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Extend narrowing to G_ASHR
ClosedPublic

Authored by arsenm on Feb 10 2020, 6:30 AM.

Diff Detail

Event Timeline

arsenm created this revision.Feb 10 2020, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 6:30 AM
foad added a subscriber: foad.Feb 10 2020, 8:01 AM
foad added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1488

"32" should be "31" here, but in any case this whole clause makes no sense to me. Why would shifting a 64-bit value right by 31 leave the low half unchanged?

1493

Why not generalize this to:

(G_ASHR i64:x, 63) for C >= 32 ->
  G_MERGE_VALUES (G_ASHR hi_32(x), C-32), (G_ASHR hi_32(x), 31)

?

foad added inline comments.Feb 10 2020, 8:03 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1493

I meant (G_ASHR i64:x, C) of course.

arsenm marked an inline comment as done.Feb 10 2020, 8:07 AM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1488

I broke this when I did a cleanup pass, this should just be HalfSize

arsenm marked an inline comment as done.Feb 10 2020, 8:09 AM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1493

I copied this from the existing AMDGPUTargetLowering::performSrlCombine, I'm not sure why this restricts it to the one shift case

arsenm updated this revision to Diff 243577.Feb 10 2020, 8:40 AM

Fix bug and generalize

foad added a comment.Feb 10 2020, 8:59 AM

Is it really worth splitting ashr into three separate cases? Can't CSE and constant folding do it all for you?

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

Low half of result should be high half of input. Both comment and code are wrong here.

1493

32 should be 63 in comment.

Is it really worth splitting ashr into three separate cases? Can't CSE and constant folding do it all for you?

I feel weird relying on such things, especially when I can see the cases. If I just delete the special case, it does not manage to take care of it

arsenm updated this revision to Diff 244058.Feb 11 2020, 7:39 PM

Fix comment

foad added a comment.Feb 12 2020, 1:27 AM

Is it really worth splitting ashr into three separate cases? Can't CSE and constant folding do it all for you?

I feel weird relying on such things, especially when I can see the cases. If I just delete the special case, it does not manage to take care of it

I guess it's up to you, but given that we already have a constant folding MIR builder it seems silly to write (and test and review) extra code to handle cases that it could handle for you. I was a bit surprised that we're not already using it. I had to tweak Combiner::combineMachineInstrs to create a ConstantFoldingMIRBuilder instead of plain MachineIRBuilder, but then your new tests still pass even if I delete the "NarrowShiftAmt == 0" special cases for all of shl, lshr and ashr.

foad added a comment.Feb 12 2020, 1:33 AM

I had to tweak Combiner::combineMachineInstrs to create a ConstantFoldingMIRBuilder instead of plain MachineIRBuilder, but then your new tests still pass even if I delete the "NarrowShiftAmt == 0" special cases for all of shl, lshr and ashr.

Sorry, forget that, I was testing the wrong thing.

foad accepted this revision.Feb 12 2020, 1:42 AM

I guess it's up to you, but given that we already have a constant folding MIR builder it seems silly to write (and test and review) extra code to handle cases that it could handle for you.

I was wrong. ConstantFoldingMIRBuilder only handles genuine constant folding like 3+4. It doesn't simplify degenerate cases like adding or shifting by 0.

LGTM.

This revision is now accepted and ready to land.Feb 12 2020, 1:42 AM
foad added inline comments.Feb 12 2020, 2:18 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1490

Actually this still needs fixing; LGTM apart from that.