This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix insert point for store rescheduling.
ClosedPublic

Authored by efriedma on Feb 17 2017, 4:18 PM.

Details

Summary

In ARMPreAllocLoadStoreOpt::RescheduleOps, LastOp should be the last operation which we want to merge. If we break out of the loop because an operation has the wrong offset, we shouldn't use that operation as LastOp.

This patch fixes some cases where we would move stores to the wrong insert point.

Now amended with fix to increment NumMove in the right place.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Feb 17 2017, 4:18 PM

Hi Eli,

Looks trivial, but would be better to have a target feature for this.

Also, would be better with an additional test where that threshold is crossed and we don't re-schedule the stores.

cheers,
--renato

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
2180

I assume this would depend on sub-architecture, so better to have a target feature, even if it's the same value for all by default, this would allow much easier fiddling and changes in the future.

would be better to have a target feature for this.

A target feature for what, exactly? This fix doesn't add any new tunable parameters. (I can add some target features for ARMPreAllocLoadStoreOpt in general in a followup.)

MatzeB edited edge metadata.Feb 21 2017, 1:05 PM

I should probably write a tutorial/blogpost about that; a good .mir test should be minimal and not just an llc -stop-after= dump, here's how to get there:

  • IR can be removed in most cases. Backreferences to IR happen with blocknames like bb.N.xxx where xxx is the IR block name can be replaced with bb.N. Memory operands keep reference to IR like (store 4 into %irvalue) they can usually be changed to (store 4) because the test doesn't need detail memory disambiguation.
  • Most yaml statements can be removed (the whole frameInfo: block here, alignment, returnsTwice, legalized, regBankSelected, selected probably)
  • There is a new feature to define the register of a vreg when writing to it: body: %0 : gpr = COPY ... instead of registers: - { id: 0, class: gpr } body: %0 = COPY ...
MatzeB accepted this revision.Feb 21 2017, 1:10 PM

The change itself looks straightforward.
Having the magic number 8 tunable would indeed be nice, unfortunately target features are only boolean (enable or disabled) today; While I would applaud everyone improving the targetfeature system to support arbitrary integer values, I don't think we need to block this bugfix for it esp. since the magic number already existed in the code.

This revision is now accepted and ready to land.Feb 21 2017, 1:10 PM

The change itself looks straightforward.
Having the magic number 8 tunable would indeed be nice, unfortunately target features are only boolean (enable or disabled) today; While I would applaud everyone improving the targetfeature system to support arbitrary integer values, I don't think we need to block this bugfix for it esp. since the magic number already existed in the code.

We already have integer values attributed to sub-architectures. But I'm not fussed about this in particular, just thought it would be a good and easy place to already fix the fixme.

The change itself looks straightforward.
Having the magic number 8 tunable would indeed be nice, unfortunately target features are only boolean (enable or disabled) today; While I would applaud everyone improving the targetfeature system to support arbitrary integer values, I don't think we need to block this bugfix for it esp. since the magic number already existed in the code.

We already have integer values attributed to sub-architectures. But I'm not fussed about this in particular, just thought it would be a good and easy place to already fix the fixme.

Ok, true. ARMSubTarget::initSubtargetFeatures() can set arbitrary values depend on the selected architecture, this may actually be useful here. What I wanted to say is that the subtarget feature APIs/tablegen do not allow you to do something like llc -mattr=MyLimit=8 yet.

What I wanted to say is that the subtarget feature APIs/tablegen do not allow you to do something like llc -mattr=MyLimit=8 yet.

Right, I'm not sure we want this at all, ever, for this particular choice.

I'm not even sure we need different values, but since we should be moving magic numbers to descriptive constants, there seems like a better place than most to put it, even if all targets use the same.

This revision was automatically updated to reflect the committed changes.
efriedma reopened this revision.Mar 1 2017, 4:59 PM

Reverted in r296718.

This revision is now accepted and ready to land.Mar 1 2017, 4:59 PM
efriedma updated this revision to Diff 90260.Mar 1 2017, 5:06 PM
efriedma edited the summary of this revision. (Show Details)

Updated patch. Move the increment of NumMove after the if statement, so we move the correct number of stores. (Hopefully I'm not making any other silly mistakes.)

efriedma updated this revision to Diff 90262.Mar 1 2017, 5:13 PM

Correctly use FileCheck in new test.

rengolin accepted this revision.Mar 2 2017, 1:32 AM

Let's try again. :)

This revision was automatically updated to reflect the committed changes.