sameerds added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:566
+ return false;
+ };
+
----------------
jdoerfert wrote:
> sameerds wrote:
> > jdoerfert wrote:
> > > jdoerfert wrote:
> > > > sameerds wrote:
> > > > > jdoerfert wrote:
> > > > > > You can use AAPointerInfo for the call site return IRPosition. It
> > > > > > will (through the iterations) gather all accesses and put them into
> > > > > > "bins" based on offset and size. It deals with uses in calls, etc.
> > > > > > and if there is stuff missing it is better to add it in one place
> > > > > > so we benefit throughout.
> > > > > I am not following what you have in mind. "implicitarg_ptr" is a
> > > > > pointer returned by an intrinsic that reads an ABI-defined register.
> > > > > I need to check that for a given call-graph, a particular range of
> > > > > bytes relative to that base pointer are never accessed. The above DFS
> > > > > on the uses conservatively assumes that such a load exists unless it
> > > > > can conclusively trace every use of the base pointer. This may
> > > > > include the pointer being passed to an extern function or being
> > > > > stored into a different memory location (although we don't expect ABI
> > > > > registers being capture this way). I am not seeing how to construct
> > > > > this around AAPointerInfo. As far as I can see, the public interface
> > > > > only talks about uses that are recognized as loads and stores.
> > > > Not actually tested, replaces the function body. Depends on D119249.
> > > > ```
> > > > const auto PointerInfoAA = A.getAAFor<AAPointerInfo>(*this,
> > > > IRPosition::callback_returned(cast<CallBase>(Ptr)),
> > > > DepClassTy::Required);
> > > > if (!PointerInfoAA.getState().isValidState())
> > > > return true; // Abort (which is weird as false is abort in the other
> > > > CB).
> > > > AAPointerInfo::OffsetAndSize OAS(*Position, /* probably look pointer
> > > > width up in DL */ 8);
> > > > return !forallInterferingAccesses(OAS, [](const AAPointerInfo::Access
> > > > &Acc, bool IsExact) {
> > > > return Acc.getRemoteInst()->isDroppable(); });
> > > > ```
> > > You don't actually need the state check.
> > > And as I said, this will take care of following pointers passed into
> > > callees or through memory to other places, all while ignoring dead code,
> > > etc.
> > I see now. forallInterferingAccesses() does check for valid state on entry,
> > which is sufficient to take care of all the opaque uses like a call to an
> > extern function or a complicated phi or a capturing store. Thanks a ton ...
> > this has been very educational!
> Yes, all "forAll" functions will return `false` if we cannot visit "all".
> Though, these methods will utilize the smarts, e.g., ignore what is dead,
> look at loads if the value is stored in a way we can track it through memory,
> transfer accesses in a callee to the caller "space" if a pointer is passed to
> the callee,... etc.
> Yes, all "forAll" functions will return `false` if we cannot visit "all".
> Though, these methods will utilize the smarts, e.g., ignore what is dead,
> look at loads if the value is stored in a way we can track it through memory,
> transfer accesses in a callee to the caller "space" if a pointer is passed to
> the callee,... etc.
@jdoerfert, do you see D119249 landing soon? We are kinda on a short runway
here (less than a handful of days) and hoping to land this review quickly
because it solves an important issue. I would prefer to have the check that you
outlined, but the alternative is to let my version through now, and then update
it when the new interface becomes available.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119216/new/
https://reviews.llvm.org/D119216
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
