[all-commits] [llvm/llvm-project] 01801d: [rs4gc] Fix a latent bug around attribute strippin...

[Philip Reames via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 01801d5274101273f85b85221392a4e2e2bc574b
      https://github.com/llvm/llvm-project/commit/01801d5274101273f85b85221392a4e2e2bc574b
  Author: Philip Reames <listmail at philipreames.com>
  Date:   2021-04-19 (Mon, 19 Apr 2021)

  Changed paths:
    M llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
    A llvm/test/Transforms/RewriteStatepointsForGC/X86/intrinsic-attributes.ll
    A llvm/test/Transforms/RewriteStatepointsForGC/X86/lit.local.cfg

  Log Message:
  -----------
  [rs4gc] Fix a latent bug around attribute stripping for intrinsics

This change fixes a latent bug which was exposed by a change currently in review (https://reviews.llvm.org/D99802#2685032).

The story on this is a bit involved.  Without this change, what ended up happening with the pending review was that we'd strip attributes off intrinsics, and then selectiondag would fail to lower the intrinsic.  Why?  Because the lowering of the intrinsic relies on the presence of the readonly attribute.  We don't have a matcher to select the case where there's a glue node needed.

Now, on the surface, this still seems like a codegen bug.  However, here it gets fun.  I was unable to reproduce this with a standalone test at all, and was pretty much struck until skatkov provided the critical detail.  This reproduces only when RS4GC and codegen are run in the same process and context.  Why?  Because it turns out we can't roundtrip the stripped attribute through serialized IR!

We'll happily print out the missing attribute, but when we parse it back, the auto-upgrade logic has a side effect of blindly overwriting attributes on intrinsics with those specified in Intrinsics.td.  This makes it impossible to exercise SelectionDAG from a standalone test case.

At this point, I decided to treat this an RS4GC bug as a) we don't need to strip in this case, and b) I could write a test which shows the correct behavior to ensure this doesn't break again in the future.

As an aside, I'd originally set out to handle libfuncs too - since in theory they might have the same issues - but backed away quickly when I realized how the semantics of builtin, nobuiltin, and no-builtin-x all interacted.  I'm utterly convinced that no part of the optimizer handles that correctly, and decided not to open that can of worms here.