This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fixup FP16 bitcasts
ClosedPublic

Authored by dmgreen on Jan 15 2020, 12:26 AM.

Details

Summary

Under fp16 we have to lower via custom lowering a bitcast.

If the bitcast is between a i32 and a f32, we currently look for the pattern we want and if we don't find it we return SDValue(), which makes the underlying DAG legaliser expand the code to a stack load and store. The i32 to f32 bitcast should still be legal though, so the whole thing has been rewritten as a dag combine.

Seems to help in some of the MVE scalarised code too.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 15 2020, 12:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 12:26 AM
dmgreen updated this revision to Diff 238186.Jan 15 2020, 12:53 AM

Update another test.

efriedma added inline comments.Jan 15 2020, 2:44 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
5709–5710

Can we make this optimization a TargetDAGCombine, instead of wedging it into legalization?

dmgreen added inline comments.Jan 15 2020, 3:23 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
5709–5710

Can you explain what you mean?

My understanding is that this is where the bug is. That we shouldn't be returning expand (SDValue()) if the node is actually legal. Trying to add something that sounds like custom bitcast lowering doesn't sound right to me.

But I may be wrong about any or all of that. Let me know.

efriedma added inline comments.Jan 15 2020, 3:44 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
5709–5710

An f32<->i32 bitcast is always legal; it shouldn't be marked "Custom" in the first place.

The transform here is to transform VMOVhr(bitcast(CopyFromReg)) -> CopyFromReg. That seems like a perfect candidate for a TargetDAGCombine.

dmgreen marked an inline comment as done.Jan 19 2020, 6:32 AM
dmgreen added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
5709–5710

Ah. Rewrite the existing code? Sure, that sounds like something that should work. And like you said might be cleaner. Let me give it a go.

dmgreen updated this revision to Diff 239059.Jan 20 2020, 3:35 AM
dmgreen edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jan 20 2020, 9:05 AM
This revision was automatically updated to reflect the committed changes.