arsenm added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:566
+ return false;
+ };
+
----------------
sameerds wrote:
> sameerds wrote:
> > jdoerfert wrote:
> > > sameerds wrote:
> > > > 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.
> > > Did you test my proposed code? I'll land my patch tomorrow, if yours
> > > works as you expect with the proposed AAPointerInfo use, great. If it
> > > doesn't I don't necessarily mind you merging something else with a clear
> > > intention to address issues and remove duplication as we go.
> > >
> > > I can lift my commit block as we go and @arsenm can also give it a
> > > good-to-go if need be.
> > Lifting your commit block would be useful in general. I certainly do not
> > intend to submit something in a hurry.
> >
> > I applied your change and tested the proposed code. It gives more
> > pessimistic results than my original crude version, i.e., it marks some
> > uses of the `implicitarg_ptr` as accessing the target range, when I am
> > pretty sure it does not. See `test_kernel42` in the lit test
> > `hsa-metadata-hostcall-v3.ll` in this review. It is intended to access
> > eight bytes from 16 onwards, which clearly does not overlap with offset 24,
> > which is the start of the target range (hostcall pointer).
> >
> > Strangely, in the debug messages, the `AAPointerInfoCallsiteReturned()`
> > constructor is called, but I don't see any messages marked [AAPointerInfo].
> Good news. The PointerInfo works as expected ... it's just that
> AMDGPUAttributor has a very short Allowed list, and I needed to add
> PointerInfo there.
>
> So @jdoerfert, D119249 works for me with the suggested changes. If that makes
> it to mainline, I'll update this change to use it.
>
> @arsenm do you see any issues with enabling AAPointerInfo in the
> AMDGPUAttributor? Will there be concerns about it being "too heavy" or
> something?
No, that's what it's there for
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
