This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by zixuan-wu on Jul 4 2022, 3:55 AM.



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.

Diff Detail

Event Timeline

zixuan-wu created this revision.Jul 4 2022, 3:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2022, 3:55 AM
zixuan-wu requested review of this revision.Jul 4 2022, 3:55 AM

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


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

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

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.


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

craig.topper added inline comments.Jul 4 2022, 7:07 PM

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

zixuan-wu added inline comments.Jul 4 2022, 7:13 PM

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

zixuan-wu updated this revision to Diff 442421.Jul 5 2022, 6:47 PM

Address comments.

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 added inline comments.Jul 5 2022, 6:54 PM

Can Cost ever be 0?

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 added inline comments.Jul 5 2022, 6:59 PM
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(),         
int ShiftedC1Cost = RISCVMatInt::getIntMatCost(                            
    ShiftedC1Int, Ty.getSizeInBits(), Subtarget.getFeatureBits(),          
// Materialising `c1` is cheaper than materialising `c1 << c2`, so the     
// combine should be prevented.                                            
if (C1Cost < ShiftedC1Cost)                                                
  return false;
zixuan-wu updated this revision to Diff 442481.Jul 6 2022, 2:37 AM
zixuan-wu retitled this revision from [RISCV] Fix the scale of getIntMatCost and also need adjust for getIntImmCost to [NFC][RISCV] Make the cost calculation of getIntMatCost more clear.
zixuan-wu edited the summary of this revision. (Show Details)

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.