Page MenuHomePhabricator

[GlobalISel] Extract a narrowScalarAddSub method. NFC
ClosedPublic

Authored by porglezomp on Jan 26 2021, 1:13 AM.

Diff Detail

Event Timeline

porglezomp created this revision.Jan 26 2021, 1:13 AM
porglezomp requested review of this revision.Jan 26 2021, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 1:13 AM
paquette added inline comments.Jan 26 2021, 9:34 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4462

Since there are only two cases, an if/else is sufficient here.

arsenm added a subscriber: arsenm.Jan 26 2021, 9:37 AM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4450

This is ignoring vectors

4494–4497

I think buildMerge does this for you

porglezomp added inline comments.Jan 27 2021, 11:15 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4462

I'm planning to expand this to support the overflowing ops later, so it will be 10 cases eventually. Should I just change it to a switch when I start adding those other cases?

aemerson added inline comments.Jan 27 2021, 11:45 PM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4462

In that case stick with the switch.

porglezomp added inline comments.Jan 29 2021, 2:37 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4450

This is what the existing code did. Re-reading it more closely, I'm beginning to think that the existing code didn't work for vectors at all—it ends up scalarizing the adds, and then doing carries between the lanes?

Running the whole LLVM test suite (with at least AMDGPU, AArch64, and X86 enabled), nothing seems to ever use the vector code path.

I think the right course here might be to remove support for vectors here, and someone can add them back properly later if they actually plan to use them.

Add a default case assertion, mark vectors as unsupported, cleanups

Existing code seemed to not support vectors correctly. I removed vector support
and re-ran the test suite on AArch64, AMDGPU, and X86, and nothing tested the
behavior of narrowScalar on a vector add, so I think the correct course of
action is to remove support here and add it back again once someone is using
it.

Moved unreachable case to bottom of switch

(I think it ends up being more readable later.)

porglezomp marked 4 inline comments as done.Jan 29 2021, 10:27 AM
arsenm added inline comments.Jan 29 2021, 10:32 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4450

I didn't realize this was just moving it. Should probably just move the code into the separate function, and separately address these other concerns

porglezomp added inline comments.Jan 29 2021, 11:02 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
4450

It's already more than just moving it because it has to change stuff to merge the add/sub case which were both written a bit differently despite doing the exact same thing, so I'm not sure where the line is exactly.

Re-add the vector code to make it more properly an NFC commit

Putting up the little change removing the vector support in a different patch.

ping, are there any changes this still needs? I think the existing comments are sufficiently addressed given that the goal is to make this an NFC change and fix them up separately.

aemerson accepted this revision.Feb 10 2021, 1:14 PM
This revision is now accepted and ready to land.Feb 10 2021, 1:14 PM
This revision was landed with ongoing or failed builds.Feb 14 2021, 3:07 PM
This revision was automatically updated to reflect the committed changes.