[PATCH] D109792: Resolve {GlobalValue,GloalIndirectSymol}::getBaseObject confusion

[Itay Bookstein via Phabricator via llvm-commits]

ibookstein added a comment.

Added a couple of comments; I think this change breaks code that wants to get the resolver Function itself because it deprives such code from the ConstantExpr-peeling machinery in findBaseObject.



================
Comment at: llvm/include/llvm/IR/GlobalIndirectSymbol.h:65
-  }
-
   const GlobalObject *getBaseObject(const DataLayout &DL, APInt &Offset) const {
----------------
When creating my own commit, I noticed that the two overloads below (with DataLayout and APInt) are completely unused. Perhaps we can use this opportunity to remove them?


================
Comment at: llvm/lib/Object/IRSymtab.cpp:289
+    if (isa<GlobalIFunc>(GV))
+      Base = cast<GlobalObject>(cast<GlobalIFunc>(GV)->getResolver())
+                 ->getBaseObject();
----------------
This will crash on bitcasts, see e.g. https://clang.godbolt.org/z/ssxrj9b7d
In other words, code that wants to deal with getting the resolver of a GlobalIFunc took a reliance upon the ConstantExpr-peeling machinery of findBaseObject() that was previously utilized only by GlobalAlias prior to the introduction of GlobalIFunc. It is unclear to me whether all the ConstantExpr handlings in findBaseObject() are needed in the case of GlobalIFunc-s.


================
Comment at: llvm/lib/Transforms/Utils/SplitModule.cpp:133
+      GVtoClusterMap.unionSets(
+          &GV, cast<GlobalObject>(GIS->getResolver())->getBaseObject());
     }
----------------
Same as above, will crash on bitcasts


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109792