Page MenuHomePhabricator

[RISCV] Add DAG combine to detect opportunities to replace (i64 (any_extend (i32 X)) with sign_extend.
ClosedPublic

Authored by craig.topper on Jun 18 2021, 4:53 PM.

Details

Summary

If type legalization is going to insert a sign_extend for other users
of X and we can fold the sign_extend into ADDW/MULW/SUBW, it is
better to replace the ANY_EXTEND so we don't end up with a separate
ADD/MUL/SUB instruction for the users of the ANY_EXTEND.

I'm only handling setcc uses right now, but there are other
instructions that force sign_extends like ashr.

There are probably other *W instructions we could use in addition
to ADDW/SUBW/MULW.

My motivating case was a loop terminating compare and a phi use
as seen in the new test file.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 18 2021, 4:53 PM
craig.topper requested review of this revision.Jun 18 2021, 4:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 4:53 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
asb accepted this revision.Jun 25 2021, 1:33 AM

Running this across the GCC torture suite I'm seeing a fair few cases where this triggers and improves codegen. This is a nice improvement, thanks!

This revision is now accepted and ready to land.Jun 25 2021, 1:33 AM
craig.topper retitled this revision from Add DAG combine to detect opportunities to replace (i64 (any_extend (i32 X)) with sign_extend. to [RISCV] Add DAG combine to detect opportunities to replace (i64 (any_extend (i32 X)) with sign_extend..Jun 25 2021, 12:54 PM
This revision was landed with ongoing or failed builds.Jun 25 2021, 11:18 PM
This revision was automatically updated to reflect the committed changes.

This patch causes an infinite loop while compiling the RISC-V Linux kernel's allmodconfig target. A simplified reproducer:

$ cat aspeed-pwm-tacho.i
char aspeed_set_pwm_port_fan_ctrl_priv_0_0,
    aspeed_set_pwm_port_fan_ctrl_fan_ctrl;
pwm_store_period, pwm_store_dc_time_on;
pwm_store() {
  pwm_store_period = aspeed_set_pwm_port_fan_ctrl_priv_0_0;
  pwm_store_period += 1;
  pwm_store_dc_time_on =
      aspeed_set_pwm_port_fan_ctrl_fan_ctrl * pwm_store_period / 5;
  if (pwm_store_dc_time_on)
    aspeed_set_pwm_port_duty_rising_falling();
}

$ timeout 5s clang -O0 --target=riscv64-linux-gnu -c -o /dev/null aspeed-pwm-tacho.i; echo $?
...
0

$ timeout 5s clang -O1 --target=riscv64-linux-gnu -c -o /dev/null aspeed-pwm-tacho.i; echo $?
...
0

$ timeout 5s clang -O2 --target=riscv64-linux-gnu -c -o /dev/null aspeed-pwm-tacho.i; echo $?
...
124

This patch causes an infinite loop while compiling the RISC-V Linux kernel's allmodconfig target. A simplified reproducer:

$ cat aspeed-pwm-tacho.i
char aspeed_set_pwm_port_fan_ctrl_priv_0_0,
    aspeed_set_pwm_port_fan_ctrl_fan_ctrl;
pwm_store_period, pwm_store_dc_time_on;
pwm_store() {
  pwm_store_period = aspeed_set_pwm_port_fan_ctrl_priv_0_0;
  pwm_store_period += 1;
  pwm_store_dc_time_on =
      aspeed_set_pwm_port_fan_ctrl_fan_ctrl * pwm_store_period / 5;
  if (pwm_store_dc_time_on)
    aspeed_set_pwm_port_duty_rising_falling();
}

$ timeout 5s clang -O0 --target=riscv64-linux-gnu -c -o /dev/null aspeed-pwm-tacho.i; echo $?
...
0

$ timeout 5s clang -O1 --target=riscv64-linux-gnu -c -o /dev/null aspeed-pwm-tacho.i; echo $?
...
0

$ timeout 5s clang -O2 --target=riscv64-linux-gnu -c -o /dev/null aspeed-pwm-tacho.i; echo $?
...
124

This should prevent the transform for this case https://reviews.llvm.org/D106754