[PATCH] D93571: [TableGen][ARM][X86] Detect combining IntrReadMem and IntrWriteMem.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D93571#2488247 <https://reviews.llvm.org/D93571#2488247>, @rcox2 wrote:

> 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 <https://reviews.llvm.org/rG7d78875f93a95815640606fa86a9972386cc5d10> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93571/new/

https://reviews.llvm.org/D93571