This is an archive of the discontinued LLVM Phabricator instance.

[TableGen][ARM][X86] Detect combining IntrReadMem and IntrWriteMem.
ClosedPublic

Authored by craig.topper on Dec 18 2020, 4:33 PM.

Details

Summary

These properties aren't additive. They are closer to ReadOnly and
WriteOnly. The default is ReadWrite. ReadMem cancels the write property and
WriteMem cancels the read property. Combining them leaves neither.

This patch checks that when we process WriteMem, the Mod flag is
still set. And for ReadMem we check that the Fef flag set still set.

I've update 2 target intrinsics that were combining these properties.

Diff Detail

Event Timeline

craig.topper created this revision.Dec 18 2020, 4:33 PM
craig.topper requested review of this revision.Dec 18 2020, 4:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2020, 4:33 PM
RKSimon accepted this revision.Dec 19 2020, 11:23 AM

LGTM

llvm/include/llvm/IR/IntrinsicsARM.td
820

maybe clean this up onto a single line now?

This revision is now accepted and ready to land.Dec 19 2020, 11:23 AM
rcox2 added a subscriber: rcox2.Jan 8 2021, 5:03 PM

Hi Craig Topper,
I noticed a regression after this change set hit our repository at Intel. The code in llvm/lib/Transforms/IPO/FunctionAttrs.cpp has a function checkFunctionMemoryAccess() that is not checking for intrinsics which may have side effects. This causes the inliner to remove a function as trivially dead when it only has a ldmxcsr instruction and some setup and shutdown code. I could change the line:

if (!AliasAnalysis::onlyAccessesArgPointees(MRB))

to

if (!AliasAnalysis::onlyAccessesArgPointees(MRB) || I->mayHaveSideEffects()) {

but I do have some concern whether this will be overly conservative.
Any thoughts?

  • Robert Cox from Intel (rcox2)

Hi Craig Topper,
I noticed a regression after this change set hit our repository at Intel. The code in llvm/lib/Transforms/IPO/FunctionAttrs.cpp has a function checkFunctionMemoryAccess() that is not checking for intrinsics which may have side effects. This causes the inliner to remove a function as trivially dead when it only has a ldmxcsr instruction and some setup and shutdown code. I could change the line:

if (!AliasAnalysis::onlyAccessesArgPointees(MRB))

to

if (!AliasAnalysis::onlyAccessesArgPointees(MRB) || I->mayHaveSideEffects()) {

but I do have some concern whether this will be overly conservative.
Any thoughts?

  • Robert Cox from Intel (rcox2)

I just commited 7d78875f93a95815640606fa86a9972386cc5d10 to drop the argmemonly attribute which I hope fixes this for you.

I think mayHaveSideEffects is an alias for "mayWriteMemory || mayThrow". It doesn't check the side effect property of the intrinsic. But it probably appears to work because mayWriteMemory ignores ArgMemOnly. So I think just dropping ArgMemOnly for this intrinsic is the better fix.