This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Add FoldBinOpIntoSelect combine
ClosedPublic

Authored by arsenm on Mar 21 2021, 7:02 AM.

Details

Summary

This will do the combine in cases that should fold, but don't
now. e.g. we're relying on the CSEMIRBuilder's incomplete constant
folding. For instance it doesn't handle FP operations or vectors (and
we don't have separate constant folding combines either to catch
them).

Diff Detail

Event Timeline

arsenm created this revision.Mar 21 2021, 7:02 AM
arsenm requested review of this revision.Mar 21 2021, 7:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2021, 7:02 AM
Herald added a subscriber: wdng. · View Herald Transcript
foad added a subscriber: foad.Mar 22 2021, 6:52 AM
foad added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
424

"truncation truncation"?

llvm/lib/CodeGen/GlobalISel/Utils.cpp
1157

Could use isNullValue here too?

arsenm added inline comments.Mar 22 2021, 11:37 AM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
1157

That is only on ConstantInt, not ConstantFP

foad added inline comments.Mar 22 2021, 12:27 PM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
1157

No, it's on Constant.

arsenm added inline comments.Mar 22 2021, 12:32 PM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
1157

Oh, that just dispatches on ConstantFP and does the same thing

paquette added inline comments.Mar 22 2021, 5:39 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3142

In the match function, you found the G_SELECT using getOpcodeDef, which walks through COPY instructions. If that walked through a COPY, I think this will give you the wrong instruction.

llvm/lib/CodeGen/GlobalISel/Utils.cpp
1135

Opcode is only used here?

Would it be hard to teach CSEMIRBuilder to handle the missing cases so that you can match top-down from the G_SELECT? It would be nice to avoid maintaining a list of constant-foldable opcodes if possible.

Would it be hard to teach CSEMIRBuilder to handle the missing cases so that you can match top-down from the G_SELECT? It would be nice to avoid maintaining a list of constant-foldable opcodes if possible.

CSEMIRBuilder won't help here, and patterns apply bottom up. Really these are all constant foldable, we just don't handle a handful of them. I don't think it's that important and this could instead use the full list of binary opcodes

arsenm updated this revision to Diff 406641.Feb 7 2022, 3:57 PM

Rebase.

Since I posted this, we now have a second version of isConstantOrConstantVector which should probably be merged

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

It's theoretically wrong but due to the copy combines, this was unobservably broken

paquette added inline comments.Feb 7 2022, 4:06 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3142

Would adding a testcase which only enables fold_binop_into_select make it possible to test?

paquette accepted this revision.Feb 7 2022, 6:23 PM

I think this looks fine

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

can pull out Select->getOpcode() into a variable

This revision is now accepted and ready to land.Feb 7 2022, 6:23 PM
foad added a comment.Feb 8 2022, 3:21 AM

For instance it doesn't handle FP operations or vectors

Is this still true? I think we do have at least some vector folds now?

foad added inline comments.Feb 8 2022, 3:31 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3099

Weird indentation?

3148

Since you claim to handle shift opcodes, you should cope with them having different LHS and RHS types.

arsenm added inline comments.Feb 8 2022, 9:46 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3148

It does cope with it, we don't actually care about the source operand types at this point. This is already covered in the tests

arsenm added a comment.Feb 8 2022, 3:18 PM

For instance it doesn't handle FP operations or vectors

Is this still true? I think we do have at least some vector folds now?

There are some vectors (plus some FP ops are handled in another one of these patches I forgot about)