[PATCH] D129072: [RISCV] Fix the scale of getIntMatCost and also need adjust for getIntImmCost

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:38
   return RISCVMatInt::getIntMatCost(Imm, DL.getTypeSizeInBits(Ty),
-                                    getST()->getFeatureBits());
+                                    getST()->getFeatureBits()) / 100;
 }
----------------
craig.topper wrote:
> 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.
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; 
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129072/new/

https://reviews.llvm.org/D129072