[PATCH] D54882: [AMDGPU] Add sdwa support for ADD|SUB U64 decomposed Pseudos

[Alex Voicu via Phabricator via llvm-commits]

AlexVlx added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:914
+  // Check if VCC is referenced in range of (MI,MISucc].
+  for (MachineBasicBlock::const_iterator I = &*std::next(MI.getIterator());
+       I != MISucc; ++I) {
----------------
ronlieb wrote:
> ronlieb wrote:
> > rampitec wrote:
> > > Ugh.. Why do you need an interator from iterator?! This may even fail if next is end(). std::next() already returns you an iterator.
> > The short answer is i have not found another mechanism to increment I to the next MI, that both compiles and does not abort.
> > 
> > In this particular context, MI and MISucc are both references to valid instructions in the basic block, and further that MISucc depends on MI,so its after MI in the instruction iteration order. 
> > 
> > This loops is not executed unless we have found both MI and MISucc, so that means we will not see MBB.end() as we will give up upon encountering MISucc.
> > 
> > Further , in practice, MI and MISucc are very close to each other, if not actually next o each other.
> > 
> > All that said, the savings from calling modifiesRegister one extra time is hopefully not too expensive in practice.
> > 
> > My preference at this point is to use one of the two following:
> > personally, i think #1 is the simplest.
> > 
> > 1) 
> > +  // Check if VCC is referenced in range of MI and MISucc.
> > +  for (MachineBasicBlock::const_iterator I = MI; I != MISucc;
> > +       I = std::next(I)) {
> > +    if (I->modifiesRegister(AMDGPU::VCC, TRI))
> > 
> > 2)
> > +  // Check if VCC is referenced in range of (MI,MISucc].
> > +  MachineBasicBlock::const_iterator I = MI;
> > +  for (++I; I != MISucc; ++I) {
> > +    if (I->modifiesRegister(AMDGPU::VCC, TRI))
> > 
> > 
> i need to use iterator instead of const_iterator
> 
>   // Check if VCC is referenced in range of (MI,MISucc].
>   for (MachineBasicBlock::iterator I = std::next(MI.getIterator());
>        I != MISucc; ++I) {
> 
Hello Ron. It is probably more hygienic (and may have spared you the pain here) to use auto in this case, since it's one of the instances where it shines. Your loop would become `for (auto I = std::next(MI.getIterator()); I != MISucc; ++I) {...}`, which also makes it robust against `MI.getIterator()` changing its return type (as unlikely as that'd be).


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

https://reviews.llvm.org/D54882