Page MenuHomePhabricator

[ARM] Enabled VMLAV and Add instructions to use VMLAVA
ClosedPublic

Authored by MeeraN on Aug 12 2020, 3:30 AM.

Details

Summary

Used InstCombine to enable VMLAV and Add instructions to generate VMLAVA instead with regression tests.

Diff Detail

Event Timeline

MeeraN created this revision.Aug 12 2020, 3:30 AM
MeeraN requested review of this revision.Aug 12 2020, 3:30 AM
dmgreen added inline comments.Aug 12 2020, 4:23 AM
llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp
216

Can you quickly run clang-format on the patch too

219

I think here you don't need the cast<Value>, if user is already an Instruction.

spatel added inline comments.Aug 12 2020, 5:16 AM
llvm/test/Transforms/InstCombine/ARM/vmldava.ll
3

This is not the right test file for this patch?

MeeraN updated this revision to Diff 285132.Aug 12 2020, 10:12 AM

Changed tests to show original IR (vmlav and add instructions) being transformed to use vmlava, run using opt -instcombine -S -mtriple=arm -o - %s instead.

dmgreen accepted this revision.Aug 13 2020, 6:16 AM

This seems good to me.

The part I am a little unsure about is that it is converting add(vmlav) into a new vmlava, but we are starting at the vmlav to do it. So need to set the insertion point and use replaceAllUsesWith/eraseInstFromFunction. I couldn't find anything else that worked like that in instcombine, but maybe just missed it. It seems to work just fine like this, but it might be cause potential problems with the way instcombine iterates? It hasn't caused any problems we have seen though.

I think this is good, but give it a day or two before committing in case anyone knows that this will be a problem.

This revision is now accepted and ready to land.Aug 13 2020, 6:16 AM

The part I am a little unsure about is that it is converting add(vmlav) into a new vmlava, but we are starting at the vmlav to do it. So need to set the insertion point and use replaceAllUsesWith/eraseInstFromFunction. I couldn't find anything else that worked like that in instcombine, but maybe just missed it. It seems to work just fine like this, but it might be cause potential problems with the way instcombine iterates? It hasn't caused any problems we have seen though.

Yes, this is not the typical code construct for instcombine. But given that we've (finally!) moved the target-specific combines out of the generic code files, I think this is the only way to match this kind of pattern.
There are similar constructs for generic patterns with phi ops and load/store, so this should be good too.