Page MenuHomePhabricator

[rs4gc] Strip memory related attributes consistently
ClosedPublic

Authored by reames on Apr 2 2021, 9:59 AM.

Details

Summary

I noticed that rs4gc is not stripping a number of memory aliasing related attributes. We do strip some from call sites, but don't strip the same ones from declarations or parameters.

Why do we need to strip these? Two answers:

  1. Safepoints conceptually read and write to the entire garbage collected heap in the physical model. We need this to preserve ordering of all loads and stores with respect to possible relocation.
  2. We can infer other attributes from these. For instance, readnone can imply both nofree and nosync. Both of which don't hold after physical rewriting.

I'm mostly posting for review to give a chance for a sanity check, and a perf test if interested. I could see this negatively impacting optimization after lowering. I doubt it's a large effect, but that might be worth measuring.

Diff Detail

Event Timeline

reames created this revision.Apr 2 2021, 9:59 AM
reames requested review of this revision.Apr 2 2021, 9:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 9:59 AM

Hi Philip, it seems this change causes functional failure.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2635

It seems that leads to functional issue.
The problem here is that this function is invoked for any declared function in a module including llvm intrinsics which are not covered by statepoint instruction.

In my failed test it appeared llvm.x86.sse2.cvttsd2si64 and as a result readnone attribute has been cleared from it.

Later it could not find this intrinsic while linking:
Cannot select: intrinsic %llvm.x86.sse2.cvttsd2si64

I landed a change (01801d5274) which should address the reported issue. (It was essentially a latent bug in the existing code.)

Once that bakes for a bit, I'd appreciate a retest cycle to see if anything else falls out. That one definitely wasn't anything I was expecting.

Once that bakes for a bit, I'd appreciate a retest cycle to see if anything else falls out. That one definitely wasn't anything I was expecting.

@skatkov Any update on this retest cycle?

Once that bakes for a bit, I'd appreciate a retest cycle to see if anything else falls out. That one definitely wasn't anything I was expecting.

@skatkov Any update on this retest cycle?

Hi Philip, sorry for the delay, testing is in progress. I'm hoping the result will be available by the end of this week.

skatkov accepted this revision.May 13 2021, 1:40 AM

Sorry for the long delay.

This revision is now accepted and ready to land.May 13 2021, 1:40 AM
This revision was landed with ongoing or failed builds.May 14 2021, 7:58 AM
This revision was automatically updated to reflect the committed changes.