This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Refactor immediate comparison instructions patterns
ClosedPublic

Authored by Chenbing.Zheng on Dec 23 2021, 2:17 AM.

Details

Summary

The patterns of the immediate comparison instruction is rewrite here, and put similar code to a class.
Do not change any function of the original code, making the code more concise.

Diff Detail

Event Timeline

Chenbing.Zheng created this revision.Dec 23 2021, 2:17 AM
Chenbing.Zheng requested review of this revision.Dec 23 2021, 2:17 AM

Is there any impack on tests ?

There are two test cases CodeGen/RISCV/rvv/vmsltu-rv32.ll and CodeGen/RISCV/rvv/vmsltu-rv64.ll failed, should re-check the pattern of vmsltu.

Is there any impack on tests ?

It should have no effect, but now there are two failure cases. I will check it.

There are two test cases CodeGen/RISCV/rvv/vmsltu-rv32.ll and CodeGen/RISCV/rvv/vmsltu-rv64.ll failed, should re-check the pattern of vmsltu.

yer, I will check it

jrtc27 added inline comments.Dec 23 2021, 5:54 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
3726

"Specil"? Assuming that's a typo for "Special" that's hardly a descriptive name

4526

This should be mapping to PseudoVMSLEU

4527

Isn't it a bit dodgy for this to be done in the first place; shouldn't we be stopping the normal int_riscv_vmsltu pattern from matching that at all? Currently this relies in SelectionDAG picking the more specific pattern, but that should solely be as a heuristic for code quality, not correctness.

Chenbing.Zheng retitled this revision from [RISCV] Refactor some comparison instructions patterns to [RISCV] Refactor immediate comparison instructions patterns.
Chenbing.Zheng edited the summary of this revision. (Show Details)
  1. Fix spelling errors and some formatting.
  2. Modify vmsltu mapping to PseudoVMSLEU and fix failed cases
craig.topper added inline comments.Dec 27 2021, 11:41 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
3726

Special -> "UnsignedZero" maybe?

craig.topper added inline comments.Dec 27 2021, 12:13 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
4527

I think we did this to match the assembler where we alias vmsltu.vi and vmsgeu.vi and with 0 immediate to vmsne and vmseq.

For vmsleu.vi I suppose we could block the 0 immediate from matching in isel and let vmsleu.vx with X0 match.

That wouldn't work for vmsgeu.vi though because there is no vmsgeu.vx instruction. We use a multiple instruction sequence. But since we know this is always true we wouldn't want to use a multiple instructions. Using vmset.m(for unmasked) and vmand.mm(for masked) would be better and avoid a false dependency I guess.

But I think any change here should not be part of this patch.

Chenbing.Zheng marked 2 inline comments as done.

Rename VPatCompareSpecial to VPatCompareUnsignedZero.

Chenbing.Zheng marked 3 inline comments as done.Dec 27 2021, 7:16 PM
Chenbing.Zheng added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
3726

done

3726

Thanks, it is a batter name.

4526

done

4527

I think what you said makes sense, but this is not something that this patch considers.

4527

Thanks! I will implement your suggestion in my next patch.

This revision is now accepted and ready to land.Dec 29 2021, 3:02 PM
This revision was landed with ongoing or failed builds.Dec 29 2021, 5:34 PM
This revision was automatically updated to reflect the committed changes.