This is an archive of the discontinued LLVM Phabricator instance.

[MachineLICM] Re-enable hoisting of constant stores
ClosedPublic

Authored by syzaara on Apr 4 2018, 1:44 PM.

Details

Summary

This patch fixes an issue exposed on the SystemZ build bots when committing https://reviews.llvm.org/rL327856. The hoisting was temporarily disabled with an option. This patch now re-enables hoisting and checks that we only hoist a store instruction when all its operands are either constant caller preserved registers or immediates.

Diff Detail

Event Timeline

syzaara created this revision.Apr 4 2018, 1:44 PM
syzaara updated this revision to Diff 141046.Apr 4 2018, 1:48 PM
  • The test could perhaps be freed of alignment, tbaa operands and dso_local specifiers, but maybe it doesn't matter. Adding Uli as reviewer since the test is in SystemZ.
  • Using isImm() but not isCImm() or isFPImm() (I don't see any method like isAnyImm() available)? It seems there may be other constant operands also, like isSymbol(), but not sure if it's useful.

Since it seems only PowerPC has implemented isCallerPreservedPhysReg() and are therefore the only user of this patch, I leave the review to the power-llvm-team. (Side question: Could the SP handling be put in common code?)

I was actually expecting to not only look for function-constant registers via isCallerPreservedPhysReg(), but to have a set of loop-invariant phys-regs for each loop (and preheader terminators). I suppose this is not needed given that the LLVM I/R LICM pass has done most of the work, or?

The test could perhaps be freed of alignment, tbaa operands and dso_local specifiers, but maybe it doesn't matter. Adding Uli as reviewer since the test is in SystemZ.

I agree it doesn't really matter. The test is fine with me as is.

  • The test could perhaps be freed of alignment, tbaa operands and dso_local specifiers, but maybe it doesn't matter. Adding Uli as reviewer since the test is in SystemZ.
  • Using isImm() but not isCImm() or isFPImm() (I don't see any method like isAnyImm() available)? It seems there may be other constant operands also, like isSymbol(), but not sure if it's useful.

Since it seems only PowerPC has implemented isCallerPreservedPhysReg() and are therefore the only user of this patch, I leave the review to the power-llvm-team. (Side question: Could the SP handling be put in common code?)

I was actually expecting to not only look for function-constant registers via isCallerPreservedPhysReg(), but to have a set of loop-invariant phys-regs for each loop (and preheader terminators). I suppose this is not needed given that the LLVM I/R LICM pass has done most of the work, or?

Thanks for the comments. The original intention of this patch was to start hoisting store instructions which are constant throughout the function, since till now there was no handling of store instructions at all. I will commit this now that the test case looks good.

jonpa added a comment.Apr 7 2018, 3:02 AM
  • The test could perhaps be freed of alignment, tbaa operands and dso_local specifiers, but maybe it doesn't matter. Adding Uli as reviewer since the test is in SystemZ.
  • Using isImm() but not isCImm() or isFPImm() (I don't see any method like isAnyImm() available)? It seems there may be other constant operands also, like isSymbol(), but not sure if it's useful.

Since it seems only PowerPC has implemented isCallerPreservedPhysReg() and are therefore the only user of this patch, I leave the review to the power-llvm-team. (Side question: Could the SP handling be put in common code?)

I was actually expecting to not only look for function-constant registers via isCallerPreservedPhysReg(), but to have a set of loop-invariant phys-regs for each loop (and preheader terminators). I suppose this is not needed given that the LLVM I/R LICM pass has done most of the work, or?

Thanks for the comments. The original intention of this patch was to start hoisting store instructions which are constant throughout the function, since till now there was no handling of store instructions at all. I will commit this now that the test case looks good.

I see. Perhaps put a comment as well that explains that there is more to do near that function..?

This revision was not accepted when it landed; it landed in state Needs Review.Apr 9 2018, 7:53 AM
This revision was automatically updated to reflect the committed changes.