Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Do we need to give a better name to the test files because the tests also check other instructions except ldptr/stptr. @xen0n Any suggestion?
In general you should name the testcases after your intent. For example for the getelementptr ..., 255 case it's evident you want to check {ld,st}ptr is not emitted for misaligned offsets, and for getelementptr ..., 4096 case you seem to be checking {ld,st}ptr is not emitted for out-of-range offsets. So names like ldptr_w, ldptr_w_misaligned_offset or ldptr_w_too_big_offset could be better conveying the intent.
Otherwise LGTM.
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td | ||
---|---|---|
952 | I'm not sure if these AddLike's could be simplified into plain add. @wangleiat any opinion? |
For getelementptr ..., 255, the offset is not 255 but 2040. It's also 4-bytes aligned. The reason why {ld,st}ptr is not generated is that {ld,st} is selected by codegen prior to {ld,st}ptr (maybe because the Pats for {ld,st} are defined before the Pats for {ld,st}ptr). AddedComplexity may change patterns' matching order.
llvm/test/CodeGen/LoongArch/ldptr.ll | ||
---|---|---|
5 | How about rename to ldptr_w_too_small_offset and write some comments for this test: ;; Check that ldptr_w is not emitted for samll offsets. | |
21 | Write some comments before this test: ;; Check that ldptr_w is emitted for applicable offsets. | |
39 | How about rename to ldptr_w_too_big_offset and write some comments for this test: ;; Check that ldptr_w is not emitted for out-of-range offsets. |
llvm/lib/Target/LoongArch/LoongArchInstrInfo.td | ||
---|---|---|
952 | So AddLike is needed due to frame index processing producing or nodes (confirmed off-line). No problem then. | |
llvm/test/CodeGen/LoongArch/ldptr.ll | ||
6 | "small" ;-) | |
62 | same here | |
66–68 | Hmm, I wonder why LLVM isn't producing ld.w $a1, $a0, 2044 then simply ld.w $a0, $a0, 2040... I don't know if there's strong requirement to read the low bits first, but we're required to split the 64-bit access into two already, so I think it's not the case. It would take another patch to fix this codegen nit though, if it's truly unexpected. No need to hurry this time. | |
llvm/test/CodeGen/LoongArch/stptr.ll | ||
6 | "small", also here. | |
59 | and here. And by the way it's better to refer to the instruction in its mnemonic form, i.e. "stptr.d", because this is a natural language sentence so we aren't forced to speak valid identifiers at all times. Please change all such occurrences. |
I'm not sure if these AddLike's could be simplified into plain add.
@wangleiat any opinion?