This is an archive of the discontinued LLVM Phabricator instance.

Allow PeepholeOptimizer to fold a few more cases
ClosedPublic

Authored by mkuper on Jul 29 2015, 6:16 AM.

Details

Summary

The current condition looks overly conservative.
I don't see why an IMPLICIT_DEF, for instance, should clear the folding candidate set, and the same for KILL.
Two things I'm not sure of:

  1. Should isPosition() be a barrier?
  2. Is the explicit check for isInlineAsm() really necessary? (InlineAsm should correctly report mayStore()/hasUnmodeledSideEffects(), right?)

Andrew, I've added you to the review because of the rdar mentioned in the test-case. The folding there currently didn't happen due to an intervening IMPLICIT_DEF, and not because of any scheduling issues. Am I missing something?

Diff Detail

Event Timeline

mkuper updated this revision to Diff 30901.Jul 29 2015, 6:16 AM
mkuper retitled this revision from to Allow PeepholeOptimizer to fold a few more cases.
mkuper updated this object.
mkuper added reviewers: qcolombet, bruno, manmanren, atrick.
mkuper added a subscriber: llvm-commits.
qcolombet edited edge metadata.Aug 6 2015, 10:17 AM

Hi Michael,

I believe the current situation is indeed an historical mistake :).

Regarding the load folding, no, I do not believe isPosition should be a barrier, probably the same for inline asm. The thing about inline asm is that if the user forgot the volatile keyword whereas it was doing unmodeled side effect, we may not model the right thing. That being said, I guess this is a mistake from the user side and we do not need to be conservative in such case.

I’d say update your patch to reflect that.

Side note, could you be more specific in the test cases regular expressions?
I.e., if you expect a memory location for a given parameter now, instead of a register, try to reflect the in the expression so that if we regress that we will see it.

Thanks,
-Quentin

atrick edited edge metadata.Aug 6 2015, 3:09 PM

I confirmed that this fixes the test case mentioned in the radar. Many issues were exposed by changing the default scheduler, but were not actually scheduling issues. Thanks for fixing it.

mkuper updated this revision to Diff 31611.Aug 9 2015, 12:57 AM
mkuper edited edge metadata.

Thanks, Quentin!
Removed the inlineAsm condition.

As to the tests - they already check for memory references. ({{.+}}) won't match a register.
In fact, this is the way the test originally was, when it checked for folding, before the scheduler breakage.

qcolombet accepted this revision.Aug 10 2015, 9:29 AM
qcolombet edited edge metadata.

As to the tests - they already check for memory references. ({{.+}}) won't match a register.

Oh, that’s right, I missed the parentheses :).

LGTM then.

Thanks,
-Quentin

This revision is now accepted and ready to land.Aug 10 2015, 9:29 AM
This revision was automatically updated to reflect the committed changes.