[PATCH] D97971: [IPSCCP] don't propagate constant in section when caller/callee sections mismatch

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1274
+      if (SectionMismatch)
+        if (auto *C = dyn_cast<Constant>(*CAI))
+          if (isGVSection(C)) {
----------------
nickdesaulniers wrote:
> fhahn wrote:
> > Shouldn't we check against the current state we have for `CAI` (using `getValueState`/ `getStructValueState`)? Otherwise we might still propagate the global into the caller, e.g. due to conditions or loads. Seeh ttps://godbolt.org/z/qMaMWE
> > 
> > Also, IIUC the real problem is the actual replacement in the function, right? If that's the case, can we just skip replacing uses in invalid functions (e.g. in `tryToReplaceWithConstant`)? That way, we would still be able to simplify conditions (see `test3` in the godbolt) and also propagate the constant through functions without the right section to their callees if they have the right section.
> > Shouldn't we check against the current state we have for CAI (using getValueState/ getStructValueState)? Otherwise we might still propagate the global into the caller, e.g. due to conditions or loads. Seeh ttps://godbolt.org/z/qMaMWE
> 
> Ah, yes! Thanks for the test case; checking the lattice for known constants actually covers your examples and my existing ones. Updated.
> 
> > Also, IIUC the real problem is the actual replacement in the function, right? If that's the case, can we just skip replacing uses in invalid functions (e.g. in tryToReplaceWithConstant)?
> 
> If you take a look at earlier revisions of this diff, that was the first approach I tried.  The issue was that at `tryToReplaceWithConstant` when we're making replacement decisions, we no longer retain information about the callsite (`CallBase`) in order to check for mismatch between caller and callee.  I used a simple vector to retain the list of arguments here in `handleCallArguments`, then scanned that list during replacement in `tryToReplaceWithConstant`.  But I found it's much less memory intensive and less work to simply detect such case when building the lattice as is done here.
> 
> We probably could go back to something closer to the original implementation; perhaps I could add some machinery to `ValueLatticeElement` to track such cases, then look for that during replacement?
> 
> > That way, we would still be able to simplify conditions (see test3 in the godbolt) and also propagate the constant through functions without the right section to their callees if they have the right section.
> 
> I see, `@test3` now returns `true` after ipsccp.  With my current change, `@cmp` from your godbolt link looks the same as the input after ipsccp is run. Hmm...
Ok, I still have some cleanup to do tomorrow (make `NonReplaceable` a private member, add comments for it, clean up function names in tests), but I think this now satisfies the additional cases you identified.  (It's remarkable to see how well SCCP replace the return value of `@test3`, from your godbolt link).  But I think this is in a good state.  (I also had a bug where the constant with an explicit section could be any member of a GEP, not necessarily the first operand, so I changed the `isGVSection` helper to DFS all operands).

This permits us to fully propagate the constants in the lattice, then simply avoid problematic replacements.

I might have more changes I'll want to do tomorrow; but if @fhahn  is ok with this approach, I'll move what I have to  `TargetTransformInfo` as per @jdoerfert 's recommendation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97971