On 15 September 2017 at 18:01, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com> wrote:
> From what I can tell Ramana and Richard preferred to encode this attribute > as > a tuning struct property rather than an inline conditional based on > arm_arch7. > I agree that if we want to use that information, it should be encoded this > way. > What I'm not convinced about is whether we do want this parameter in the > first place. > > The cost tables already encode information about the costs of different > sized loads/stores. > In patch 2, for example, you add the cost for extra_cost->ldst.load which is > nominally just > the cost of a normal 32-bit ldr. But we also have costs for ldst.ldrd which > is the 64-bit two-register load > which should reflect any extra cost due to a narrower bus in it. We also > have costs for ldst.loadf (for 32-bit > VFP loads) and ldst.loadd (for 64-bit VFP D-register loads). So I think we > should use those cost fields > depending on the mode class and size instead of using ldst.load > unconditionally and adding a new bus_size parameter. > > So I think the way forward is to drop this patch and modify patch 2/3 to use > the extra_cost->ldst fields as described above. > > Sorry for the back-and-forth. I think this is the best approach because it > uses the existing fields more naturally and > doesn't add new parameters that partly duplicate the information encoded in > the existing fields. > Ramana, Richard: if you prefer the bus_width approach I won't block it, but > could you clarify your preference? > If we do end up adding the bus_width parameter then this patch and patch 2/3 > look ok. > Thanks, > Kyrill > > P.S. I'm going on a 4-week holiday from today, so I won't be able to do any > further review in that timeframe. > As I said, if we go with the bus_size approach then these patches are ok. If > we go with my suggestion, this would > be dropped and patch 2 would be extended to select the appropriate > extra_cost->ldst field depending on mode. OK, I agree with dropping this patch. I have posted an updated patch 2 which does not require it.
