[PATCH] D55444: AMDGPU: Fix DPP combiner

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin marked an inline comment as done.
vpykhtin added inline comments.


================
Comment at: lib/Target/AMDGPU/GCNDPPCombine.cpp:476
       BB->insert(OrigMI, NewMI);
       if (TII->commuteInstruction(*NewMI)) {
         LLVM_DEBUG(dbgs() << "  commuted:  " << *NewMI);
----------------
arsenm wrote:
> arsenm wrote:
> > vpykhtin wrote:
> > > vpykhtin wrote:
> > > > nhaehnle wrote:
> > > > > Can we simplify the code here by just commuting the OrigMI unconditionally?
> > > > > 
> > > > > That is, first check whether `&Use != src0 && &Use == src1`; in that case, try commuting the instruction. If that fails, break (but I don't think there'd be a need to rollback the commutation). If it succeeds, continue down the same path that is used for the `&Use == src0` case. You end up with only a single call to `createDPPInst` and less code duplication in general.
> > > > This is a good idea: I don't like leaving commuted instruction on rollback at some intuitive level but really don't have arguments against it at the moment :-)
> > > Actually a pass returns boolean meaning it changes something in the IR. What should it return when it does rollback? It may create new commuted instructions which are need to be mapped in SlotIndexes for example. I don't think its a good idea to return true only when a rollback took place.
> > Can rollback be avoided?
> > 
> > I think the usual purpose of knowing something changed is to know if iterators are still valid, which probably isn't the case if anything was modified at any point
> I suppose if it's just commuting, it's ok to report no change
The commutation is the whole problem. I cannot know whether the instuction can be commuted beforehand. Also an instruction copy should be created for the commutation - this means that  previous analisys should be invalidated and the IR change should be repored.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55444