[PATCH] D139394: [RISCV] Add support for RISCV XVentanaCondOps Extension Patterns

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/test/CodeGen/RISCV/select.ll:287
+;
+; CONDOPS-LABEL: select_add_1:
+; CONDOPS:       # %bb.0: # %entry
----------------
craig.topper wrote:
> craig.topper wrote:
> > reames wrote:
> > > We should be able to do better here since 0 is the identity constant for the add.
> > > 
> > > We should be able to lower this as (add (select cond, a, 0), b).  There should only need a single vt.maskc here.
> > InstCombine already does that transform before we get to SelectionDAG so it might not be an issue in practice.
> Note there are two operations built into the addw. An add and a sign extension.
> 
> Doing a peephole on this in isel would require knowing that the promoted version of %b has 33 sign bits so that `addw %b, 0` was equivalent to %b. It doesn't have 33 sign bits because it doesn't have the `signext` attribute.
> 
> It would also be valid if we knew the upper bits of the result didn't matter. But we have lost that information by the time we reach isel.
> 
> Alternatively, we could rewrite it before type legalization if we thought it was important.
When making the original comment I had not realized this was specific to i32, and that the i64 case looks better below.

Hm, this points out a testing issue here.  All of the existing tests are i32, and the added ones are i64.  We should probably merge the test files so that the native width codegen is visible with and without.

To be clear, this is not meant to be a blocking comment.  More making observations on potential improvements.   


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

https://reviews.llvm.org/D139394