This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Don't use constantpool for floating-point value if the value can be easily constructed by integer sequence and a floating-point move.
ClosedPublic

Authored by HanKuanChen on Jan 30 2023, 10:55 PM.

Details

Summary

In addition, this commit does the following combine

vfmv.v.f + fmv.[dhw].x -> vmv.v.x
vfmv.s.f + fmv.[dhw].x -> vmv.s.x
vfmerge.vfm + fmv.[dhw].x -> vmerge.vxm

Diff Detail

Event Timeline

HanKuanChen created this revision.Jan 30 2023, 10:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 10:55 PM
HanKuanChen requested review of this revision.Jan 30 2023, 10:55 PM
craig.topper added inline comments.Jan 31 2023, 4:50 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
713

Drop Scalar from this. It's never a vector. Nor is it an Elt

721

Why is the isZero check needed? There was an early exit for positive zero so this could only be negative 0. But a negative 0 larger than xlen wouldn't be able to generate a correct constant.

Allen added a subscriber: Allen.Jan 31 2023, 4:52 PM

Apply Craig's comment.

asb added a comment.Feb 1 2023, 6:26 AM

Was the motivation for adding the combines mentioned in the patch description to prevent a regression due to this change? In the absence of that, we'd typically try to keep each patch as minimal as possible.

I'm curious if anyone has concerns about cases where converting an int to float might be more expensive that isn't captured in the current cost modelling - from a quick look it does seem likely to be a win unless the int/fp conversion cost is particularly high. Are there other targets that make this same choice? From a quick look at Arm it appears to use an int->float move when doing execute-only codegen but otherwise. Of course it has a wider set of options available in that ISA for efficiently materialising FP constants.

Was the motivation for adding the combines mentioned in the patch description to prevent a regression due to this change? In the absence of that, we'd typically try to keep each patch as minimal as possible.

I'm curious if anyone has concerns about cases where converting an int to float might be more expensive that isn't captured in the current cost modelling - from a quick look it does seem likely to be a win unless the int/fp conversion cost is particularly high. Are there other targets that make this same choice? From a quick look at Arm it appears to use an int->float move when doing execute-only codegen but otherwise. Of course it has a wider set of options available in that ISA for efficiently materialising FP constants.

Without the vector isel changes, we'd do scalar integer lui -> scalar int to scalar fp move -> scalar fp to vector. The vector isel changes move the integer directly to vector.

asb added a comment.Feb 1 2023, 9:38 AM

Oh of course - that makes sense, thanks.

I generally support this change once normal review completes. I don't *think* this is likely to be too painful on any CPUs I'm currently aware of, but if it is, having it under a cpu tuning flag at a minimum makes sense. I'd been thinking about this myself a bit in the background, and this patch looks basically identical to what I'd settled on heuristic wise. This does kick in more than I'd expected. Apparently, I'd misjudged how many floats this would actually pickup.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
729

default: llvm_unreachable("message");

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1477

I think this would be more clear if FPImmCost default to two (since the constant pool load uses two instructions), and you replace the <= with <. Same result, but more obvious (to me).

llvm/test/CodeGen/RISCV/float-round-conv.ll
131–132

Not directly related to this patch, but these test diffs make it look like a constant pool load is blocking stack setup/tear down optimization. (Sorry, there's a particular name for this optimization, but my brain is blanking right now.) We should probably fix that separately.

craig.topper added inline comments.Feb 2 2023, 12:33 PM
llvm/test/CodeGen/RISCV/float-round-conv.ll
131–132

Shrink wrap?

reames added inline comments.Feb 2 2023, 12:35 PM
llvm/test/CodeGen/RISCV/float-round-conv.ll
131–132

Yep, that was it.

Apply Philip's comment.

reames accepted this revision.Feb 3 2023, 7:41 AM

LGTM, but please wait a day or two before landing in case @craig.topper has comments. I still don't consider myself to fully understand the ISEL interactions. I *think* this is good to go, but probably good to have him double check.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1471

Doing the -0.0 case as an early return check would likely be clearer here.

This revision is now accepted and ready to land.Feb 3 2023, 7:41 AM
craig.topper accepted this revision.Feb 3 2023, 6:30 PM

LGTM. I had already looked at this internally at SiFive and given some guidance.

FYI, I found RV32 vector crash after this patch. Fixed in 712e143883d694d3b5817dae714da2315eae8c89