BSET promotion too aggressive?

Code Generation RISCV
1

There is some code in RISCVISelLowering.cpp:

// If we can use a BSET instruction, allow default promotion to apply.
      if (N->getOpcode() == ISD::SHL && Subtarget.hasStdExtZbs() &&
          isOneConstant(N->getOperand(0)))
        break;

The defaults to BSET promotion given Zbs extension support.

Given this test case:

int getBit(int a) {
    return 0x1 << (a % 32);
}

The IR is:

define dso_local signext i32 @getBit(i32 noundef signext %a) local_unnamed_addr #0 {
entry:
  %rem1 = and i32 %a, 31
  %shl = shl nuw i32 1, %rem1
  ret i32 %shl
}

And the asm looks like:

# %bb.0:                                # %entry
  andi  a0, a0, 31
  bset  a0, zero, a0
  sext.w  a0, a0
  ret

instead of:

# %bb.0:                                # %entry
  li  a1, 1
  sllw  a0, a1, a0
  ret

Should we be making exceptions? @topperc

1 Like
2

This looks like a great minimal test case and a real issue - filing things like this as a bug is always a good way of ensuring it’s properly tracked. Thanks for spotting the problem.

3

I was just curious if this had been noticed and it was decided if bset promotion by default was worth it anyways as this is a small test case. I’ll look into a fix. I’m not sure how it would affect tests people might be running if we simply removed this and handled bset opts differently.

4

I really wish we’d gotten a bsetw instruction. The current hack works well when there is no mask on the shift amount and the upper bits of the result aren’t used by later instructions. If we let it become RISCVISD::SLLW we can’t turn it into BSET without adding an andi and a sext.w.

Maybe we can write an isel pattern for (sext (shl 1, (and X, 31))) and use sllw (li 1), X in that case.

5

If this is a corner case, we could default to bset and write a pattern to go back to sllw.