Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
It is not clear for me how my code failed the "pre-merge checks", can someone help to point out what's the problem? Thanks.
Shouldn't this test just go in imm.ll?
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 148 | This would be clearer written in binary (with ... for the long sequence of 1's) and x's for the interesting bits | |
| 149 | Doesn't this work whenever LeadingOnes + TrailingOnes > (64 - 12) and TrailingOnes > 0 (which is currently implied by your conditions, somewhat non-obviously)? I don't see why you need LeadingOnes < 32, other than to avoid the case when the input is all 1's or all 0's and you end up trying to shift by 64, i.e. also TrailingOnes < 64. | |
| 150 | Need to cast Val to uint64_t when left-shifting too otherwise it's UB; left-shifting on signed types is only defined if the LHS is non-negative and LHS * 2^RHS computed as a natural number is representable in the integral type used, i.e. you're only shifting out 0s at the top and there's at least one left over. | |
| 152 | Just return true | |
| 154 | With early return you can avoid the nesting | |
| 155 | This is UB in a lot of cases, left shifts on signed values are perilous | |
| 157–158 | Again this seems overly strict, you just need LowerLeadingOnes + UpperTrailingOnes > (64 - 12) and 0 < UpperTrailingOnes < 32? | |
| 162 | Ditto | |
| 165 | Just return false | |
Thanks for helping on review.
Please help to check if there is still any issue with new version.
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 157–158 | Yes, I agree we should only use "LowerLeadingOnes + UpperTrailingOnes > (64 - 12)". For 0 < UpperTrailingOnes < 32, it seems we don't need it, as: | |
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 149 | As I said you need to take care when the input is all 1s, i.e. check say TrailingOnes < 64, otherwise you have UB due to shifting a uint64_t by 64 | |
| 157–158 | I don't like relying on that, it's fragile, and not obvious because nowhere does it say that's a condition. It should at least be an assertion. Except, once you correctly handle all-ones input for the first case, i.e. don't let it match as that would cause UB in the shifts, you will end up falling through to here, and you do need to explicitly check UpperTrailingOnes < 32, otherwise this will be broken too. | |
Thanks for helping review on this.
Please help to check if there is still any issue.
| llvm/test/CodeGen/RISCV/imm.ll | ||
|---|---|---|
| 13 | You've clearly not regenerated this file, just hacked it up by hand; use the script. Also probably better to put Zbb between Zba and Zbs? | |
| llvm/test/CodeGen/RISCV/imm.ll | ||
|---|---|---|
| 13 | I wasn't sure how to use the script. | |
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 337 | You could return Rotate from the function instead of a bool and use 0 to mean no rotate. And instead of having NegImm12 as an output parameter, you could use the returned Rotate amount to rotate Val left in this if block before using it in the ADDI. That way you wouldn't have two different places in extractRotateInfo rotating Val left to commute NegImm12. | |
| llvm/test/CodeGen/RISCV/imm.ll | ||
| 2330 | Insert blank between test functions | |
| 2366 | Add blank line | |
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 337 | Err... that should have said "compute" not "commute" | |
Please help to check the code again. Thanks.
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 337 | Great idea. The code is much simpler now. | |
| llvm/test/CodeGen/RISCV/imm.ll | ||
|---|---|---|
| 13 | Did you regenerate the CHECK lines before reordering the RUN lines? Because the order doesn't match, and I suspect a subsequent regenerate will result in all the CHECK lines moving, making it hard to see actual future diffs. | |
| llvm/test/CodeGen/RISCV/imm.ll | ||
|---|---|---|
| 13 | Ok, I regenerated the file, now the order looks correct. | |
I don't understand why my change would result such error:
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm -- Exit Code: 1 Command Output (stderr): -- no required module provides package llvm.org/llvm/bindings/go/llvm: go.mod file not found in current directory or any parent directory; see 'go help modules'
I also tried to run this test locallly on my ubuntu host but got such result:
./bin/llvm-lit /workspace/risc-v/bpang/T_E_S_T/llvm-project/llvm/test/Bindings/Go/go.test
-- Testing: 1 tests, 1 workers --
UNSUPPORTED: LLVM :: Bindings/Go/go.test (1 of 1)
Testing Time: 0.07s
Unsupported: 1The Go binding tests are known to be flaky, you can ignore that, it's totally unrelated
The Go binding tests are known to be flaky, you can ignore that, it's totally unrelated
Then what else should I do to move forward for this review?
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 337 | Use int64_t here so that no bits are dropped. That will make the assert isInt<12> assert more accurate too. | |
| llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp | ||
|---|---|---|
| 338 | Not here. On the declaration of NegImm12. Right now you have uint64_t->int->int64_t. I'm suggesting uint64_t->int64_t. | |
@craig.topper
Thanks for the reivew.
I forget to mention that I don't have push permission yet, Can you help to push my code in? Thanks.
Yes, please provide your name and email as you would like it to appear in the git log
Yes, please provide your name and email as you would like it to appear in the git log
Baoshan Pang
pangbw@gmail.com
Thanks.
Use unsigned instead of int