[PATCH] D35049: LSR tunings for SystemZ, with some minor common code changes

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa closed this revision.
jonpa marked 3 inline comments as done.
jonpa added a comment.

Thanks for review!
r308729



================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:643
   // Indexing is OK but no scale factor can be applied.
   return AM.Scale == 0 || AM.Scale == 1;
 }
----------------
uweigand wrote:
> Just as a minor readability enhancement, I'd move the supportedAddressingMode check down here, and write the whole thing like this:
> 
> ```
> if (I != nullptr && !supportedAddressingMode(I, Subtarget.hasVector()).IndexReg)
>   // No indexing allowed.
>   return AM.Scale == 0;
> else
>   // Indexing is OK but no scale factor can be applied.
>   return AM.Scale == 0 || AM.Scale == 1;
> ```
Done.


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:651
 
   return true;
 }
----------------
uweigand wrote:
> Hmm.  I know the code didn't before either, but shouldn't we check whether the Offset fits into 20 bits here?  Maybe at least add a TODO if we don't want to change it right now.
I think my idea was that isLegalAddressingMode() would handle that during formula generation. I anyhow added a TODO and will check this for the next patch. 


https://reviews.llvm.org/D35049