This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Implement G_SELECT translate to min/max/abs
AbandonedPublic

Authored by yuntaopan on Mar 7 2022, 12:00 AM.

Details

Summary

We are porting gisel to our backend, but found that some optimization capabilities are weaker than SDISel. This is one of the optimizations G_SELECT translate to min/max/abs from SDISel

Diff Detail

Event Timeline

yuntaopan created this revision.Mar 7 2022, 12:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 12:00 AM
yuntaopan requested review of this revision.Mar 7 2022, 12:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 12:00 AM
yuntaopan updated this revision to Diff 413355.Mar 7 2022, 12:10 AM
This comment was removed by yuntaopan.
yuntaopan edited the summary of this revision. (Show Details)Mar 7 2022, 12:16 AM
aemerson added subscribers: foad, arsenm.

Thanks for the patch. Unfortunately I see the code that this was ported from in SelectionDAGBuilder does the optimization there because it relies on ValueTracking's matchSelectPattern() infrastructure. @arsenm @foad do you think it's worth it here to replicate that functionality for generic MIR so we can avoid adding this to the IRTranslator?

foad added a comment.Mar 8 2022, 1:18 AM

Thanks for the patch. Unfortunately I see the code that this was ported from in SelectionDAGBuilder does the optimization there because it relies on ValueTracking's matchSelectPattern() infrastructure. @arsenm @foad do you think it's worth it here to replicate that functionality for generic MIR so we can avoid adding this to the IRTranslator?

Shouldn't the select -> min/max/abs be done in IR, so there would be no need for this patch? See D98152 for example.

IIRC I was working on something similar in the combiner but for FP stuff (D116702)

Thanks for the patch. Unfortunately I see the code that this was ported from in SelectionDAGBuilder does the optimization there because it relies on ValueTracking's matchSelectPattern() infrastructure. @arsenm @foad do you think it's worth it here to replicate that functionality for generic MIR so we can avoid adding this to the IRTranslator?

Shouldn't the select -> min/max/abs be done in IR, so there would be no need for this patch? See D98152 for example.

Our development branch does not have that patch yet, I have tried that patch and it works well. it's more appropriate to handle it at the IR and no need for this patch. But I have a question,why gisel can not rely on ValueTracking infrastructure ? Because gisel is based on mir, but ValueTracking is an IR analysis ?@aemerson

Thanks for the patch. Unfortunately I see the code that this was ported from in SelectionDAGBuilder does the optimization there because it relies on ValueTracking's matchSelectPattern() infrastructure. @arsenm @foad do you think it's worth it here to replicate that functionality for generic MIR so we can avoid adding this to the IRTranslator?

Shouldn't the select -> min/max/abs be done in IR, so there would be no need for this patch? See D98152 for example.

Our development branch does not have that patch yet, I have tried that patch and it works well. it's more appropriate to handle it at the IR and no need for this patch. But I have a question,why gisel can not rely on ValueTracking infrastructure ? Because gisel is based on mir, but ValueTracking is an IR analysis ?@aemerson

There's nothing wrong with using the ValueTracking infrastructure, the problem is that the only place we can use IR analysis are during the IRTranslator phase, and we try to avoid doing optimizations there as much as possible. Sometimes that might not be practical, but where we can do like to perform optimizations on generic MIR in combine passes instead.

arsenm requested changes to this revision.Aug 17 2023, 3:49 PM

Is this still relevant?

I think a native min/max matching combiner would be more helpful, the pattern can easily appear during legalization. InstCombine forms min/max intrinsics these days, so producing them at the start of codegen isn't really necessary

This revision now requires changes to proceed.Aug 17 2023, 3:49 PM
yuntaopan abandoned this revision.Aug 17 2023, 8:11 PM

Related optimizations have been done in InstCombine Pass, so there is no need to support such optimizations here