Page MenuHomePhabricator

[AArch64][GlobalISel] When lowering signext i1 parameters, don't zero-extend to s8 first.
ClosedPublic

Authored by aemerson on Sep 5 2022, 2:05 PM.

Details

Summary

We were generating different code to SelectionDAG, trying to extend to s8 first as per ABI instead of honoring the IR signext attribute first.

More discussion at https://github.com/llvm/llvm-project/issues/57181

Diff Detail

Unit TestsFailed

TimeTest
60,030 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

Event Timeline

aemerson created this revision.Sep 5 2022, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 2:05 PM
aemerson requested review of this revision.Sep 5 2022, 2:05 PM
aemerson edited the summary of this revision. (Show Details)
miyuki added a comment.Sep 6 2022, 7:36 AM

Could you please add tests for signext/zeroext i1 return type?

Could you please add tests for signext/zeroext i1 return type?

Sure, that also needed fixing.

aemerson updated this revision to Diff 458185.Sep 6 2022, 8:43 AM

Fix return attribute case too.

Allen added a subscriber: Allen.Sep 6 2022, 8:49 AM
paquette added inline comments.Sep 6 2022, 10:07 AM
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
387

should we just pull CurArgInfo.Flags[0] out into a variable?

574–575

ditto with OrigArg.Flags[0]?

You said We were generating different code to SelectionDAG, Is there a test file that shows the generated code of SelectionDAG and GlobalIsel?

aemerson updated this revision to Diff 458666.Sep 8 2022, 12:06 AM

Address comments.

You said We were generating different code to SelectionDAG, Is there a test file that shows the generated code of SelectionDAG and GlobalIsel?

I added run lines to bool-ext-inc.ll and the tests that were used to generate the MIR tests too.

You said We were generating different code to SelectionDAG, Is there a test file that shows the generated code of SelectionDAG and GlobalIsel?

I added run lines to bool-ext-inc.ll and the tests that were used to generate the MIR tests too.

Thanks! The SelectionDAG code seems to be shorter (#instructions).

You said We were generating different code to SelectionDAG, Is there a test file that shows the generated code of SelectionDAG and GlobalIsel?

I added run lines to bool-ext-inc.ll and the tests that were used to generate the MIR tests too.

Thanks! The SelectionDAG code seems to be shorter (#instructions).

Yes, code quality there needs work still.

I'd forgotten this hasn't landed yet. Given the comments were just about tests, I'll commit this now. Feel free to respond with any further issues and I'll address them post-commit.

aemerson accepted this revision.Oct 15 2022, 8:26 PM
This revision is now accepted and ready to land.Oct 15 2022, 8:26 PM