Origianl patch made a mistake that ugt is reverse cc should be ule.
And ule < C will be generalize to ult < C + 1. So the new patch add support for ult < Pow2 case.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19836 | Do you know if CCIndex and CmpIndex always have consistent values? If so it would be cleaner and faster to not use a loop and instead initialise the Ops vector with the correct values in the correct places. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19836 | Based on current code, CCIndex should be always 2, CmpIndex should be always 3. I'm not sure it will change or not in the future. But I can try to use the cleaner way and add a assert here. |
llvm/test/CodeGen/AArch64/andcompare.ll | ||
---|---|---|
2406 | I'm sorry but I haven't understand what you said. What labels are mixed up? One of the result is SDAG and the other is GISel. |
llvm/test/CodeGen/AArch64/andcompare.ll | ||
---|---|---|
2406 | The IR function is called cmp_to_ands1 but the check statement is looking for cmp_to_subs1, that's why the test is failing: https://reviews.llvm.org/harbormaster/unit/view/5713977/ It's the same for the other IR functions below. |
llvm/test/CodeGen/AArch64/andcompare.ll | ||
---|---|---|
2406 | Ah, thanks. I will update it soon. |
Thanks for this, it's looking great, with one small suggestion.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19839 | I'd reword the assertion message to be more like "Expected CCIndex to be 2 and CmpIndex to be 3", to make it more like existing assertions. The part about updating it would be better in a comment, in my opinion. |
It miscompiled CodeGen/TargetLoweringObjectFileImpl.cpp
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19810 | I think the latter is wrong. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19810 | Thanks for the case. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19810 | I reconfirmed the new expr is correct. Could you enhance to add also them? (X & C) >=u Pow2 |
Not handled by instcombine is because AArch64 has tst instruction that instcombine need two instruction to do that.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19810 | We needn't enhance the case in the code. DAGCombiner will generalize these to current two patterns. Check the test: |
Will this affect other tests? I met another.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
19810 | Thanks, I understood. |
I don't know actually. The motivation of this change is fix https://github.com/llvm/llvm-project/issues/59598.
Maybe @samtebbs can answer this question.
I think the latter is wrong.
eg. X=11 C=7 M=3