This is an archive of the discontinued LLVM Phabricator instance.

Reapply [AArch64] fold subs ugt/ult to ands when the second operand is mask/pow2
ClosedPublic

Authored by bcl5980 on Jan 16 2023, 1:41 AM.

Details

Summary

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.

https://alive2.llvm.org/ce/z/naBw5A

Diff Detail

Event Timeline

bcl5980 created this revision.Jan 16 2023, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 1:41 AM
bcl5980 requested review of this revision.Jan 16 2023, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 1:41 AM
samtebbs added inline comments.Jan 16 2023, 3:51 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19843

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.

bcl5980 added inline comments.Jan 16 2023, 5:43 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19843

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.

samtebbs added inline comments.Jan 16 2023, 5:50 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19843

That's a good idea, thanks. I think accounting for how things are at the moment and tripping an assertion if it changes is a good approach.

llvm/test/CodeGen/AArch64/andcompare.ll
2406

Looks like the labels in the check statements are mixed up.

bcl5980 added inline comments.Jan 16 2023, 5:55 AM
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.

samtebbs added inline comments.Jan 16 2023, 5:57 AM
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.

bcl5980 added inline comments.Jan 16 2023, 5:58 AM
llvm/test/CodeGen/AArch64/andcompare.ll
2406

Ah, thanks. I will update it soon.

bcl5980 updated this revision to Diff 489526.Jan 16 2023, 6:04 AM

address comments.

samtebbs accepted this revision.Jan 16 2023, 6:10 AM

Thanks for this, it's looking great, with one small suggestion.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19846

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.

This revision is now accepted and ready to land.Jan 16 2023, 6:10 AM
bcl5980 updated this revision to Diff 489682.Jan 16 2023, 8:00 PM

update comments.

This revision was landed with ongoing or failed builds.Jan 16 2023, 8:02 PM
This revision was automatically updated to reflect the committed changes.

Seems it broke aarch64 bootstrap. Investigating.

It miscompiled CodeGen/TargetLoweringObjectFileImpl.cpp

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19817

I think the latter is wrong.
eg. X=11 C=7 M=3

bcl5980 added inline comments.Jan 17 2023, 2:23 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19817

Thanks for the case.
I will send a quick fix for that.

bcl5980 reopened this revision.Jan 17 2023, 7:26 PM
bcl5980 updated this revision to Diff 490021.
bcl5980 retitled this revision from [AArch64] fold subs ugt/ult to ands when the second operand is a mask to Reapply [AArch64] fold subs ugt/ult to ands when the second operand is a mask.
bcl5980 edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jan 17 2023, 7:26 PM
bcl5980 retitled this revision from Reapply [AArch64] fold subs ugt/ult to ands when the second operand is a mask to Reapply [AArch64] fold subs ugt/ult to ands when the second operand is mask/pow2.Jan 17 2023, 7:34 PM
chapuni added inline comments.Jan 17 2023, 8:40 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19817

I reconfirmed the new expr is correct.
(not tested yet for bootstrap, though)

Could you enhance to add also them?

(X & C) >=u Pow2
(X & C) <=u Mask

btw, was it not handled by instcombine?

btw, was it not handled by instcombine?

Not handled by instcombine is because AArch64 has tst instruction that instcombine need two instruction to do that.

bcl5980 updated this revision to Diff 490039.Jan 17 2023, 10:17 PM

update two more tests for:
(X & C) >=u Pow2
(X & C) <=u Mask

bcl5980 added inline comments.Jan 17 2023, 10:19 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19817

We needn't enhance the case in the code. DAGCombiner will generalize these to current two patterns. Check the test:
(X & C) >=u Pow2 in cmp_to_ands6
(X & C) <=u Mask in cmp_to_ands4

Will this affect other tests? I met another.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19817

Thanks, I understood.

Will this affect other tests? I met another.

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.

bcl5980 updated this revision to Diff 490061.Jan 18 2023, 12:21 AM

Fix tests failure in CodeGen/AArch64/and-mask-removal.ll

chapuni accepted this revision.Jan 18 2023, 1:47 AM

Works fine from my side, thanks!

This revision was landed with ongoing or failed builds.Jan 18 2023, 3:24 AM
This revision was automatically updated to reflect the committed changes.