[PATCH] D55444: AMDGPU: Fix DPP combiner

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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:
> 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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55444