[NFC][RISCV] Make the cost calculation of getIntMatCost more clear

[zixuan-wu]

Summary

The getIntMatCost function calculates the cost of instruction sequence in two different scale factor, which depends on the bool CompressionCost argument. The getInstSeqCost function uses 100 as multiply factor value, no matter RVC is enable.

Then, getIntMatCost is used in getIntImmCost function with CompressionCost = false to get num of instructions to get Imm cost. And it's also used in isDesirableToCommuteWithShift with CompressionCost = true to get the cost with balancing the codesize benefit of RVC.

[craig.topper]

I’m not sure there was any bug here. See comment.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp

384

HasRVC also considers ‘CompressionCost’ so it only scaled the value when that was passed. TargetTransformInfo doesn’t set that flag.

[liaolucy]

I would like to ask a question, is this patch to reduce code size?

[zixuan-wu]

In D129072#3629019, @liaolucy wrote:

I would like to ask a question, is this patch to reduce code size?

I think the instruction cost model can be used both in code size and performance. Normally it's in TCK_SizeAndLatency kind.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp

384

I think it's better to use same multiplicative factor in one cost function, no matter the RVC is enable or not.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp

398–402

Doesn’t this prevent the function from being able to return 75 like it should for a single compressed instruction?

[zixuan-wu]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp

398–402

Right. I think we can just return the Cost directly.

[zixuan-wu]

Address comments.

[zixuan-wu]

I found it's no change in .ll test cases. I am wondering if there should be a cost model test case like llvm/test/Analysis/CostModel/ARM/immediates.ll, or make it as NFC patch.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp

398–402

Can Cost ever be 0?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

38 ↗

(On Diff #442421)

This is now an assumption that the cost is >= 100 or it will produce 0. This is why I don't really like this patch. The compressed cost with its 75 and 100 only make sense for a relative comparison not an absolute cost.

Perhaps the real problem is that getIntMatCost is the external interface we use for both cases. Maybe we need a getIntMatSize for the one case that cares about compressed size.

[craig.topper]

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp

38 ↗

(On Diff #442421)

Another option is to expose a function that takes 2 immediates and returns if one is cheaper. That would encapsulate this code

// Neither constant will fit into an immediate, so find materialisation    
// costs.                                                                  
int C1Cost = RISCVMatInt::getIntMatCost(C1Int, Ty.getSizeInBits(),         
                                        Subtarget.getFeatureBits(),        
                                        /*CompressionCost*/true);          
int ShiftedC1Cost = RISCVMatInt::getIntMatCost(                            
    ShiftedC1Int, Ty.getSizeInBits(), Subtarget.getFeatureBits(),          
    /*CompressionCost*/true);                                              
                                                                           
// Materialising `c1` is cheaper than materialising `c1 << c2`, so the     
// combine should be prevented.                                            
if (C1Cost < ShiftedC1Cost)                                                
  return false;

[zixuan-wu]

The logic of getIntMatCost and getInstSeqCost is correct to be called in different code place. But the logic is not clear.
So make it clear without still constructing 2 different version of getIntMatCost to reuse the for loop to calculate cost of each chunk.