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

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

Details

Reviewers

efriedma
paquette
miyuki
t.p.northover
jroelofs
aemerson

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

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	60,030 ms	x64 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

Herald added subscribers: hiraditya, kristof.beyls, rovka. · View Herald Transcript

aemerson requested review of this revision.Sep 5 2022, 2:05 PM

aemerson edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B185145: Diff 458055.Sep 5 2022, 3:32 PM

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

miyuki added a subscriber: john.brawn.Sep 6 2022, 8:15 AM

In D133327#3772046, @miyuki wrote:

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

Sure, that also needed fixing.

Fix return attribute case too.

Allen added a subscriber: Allen.Sep 6 2022, 8:49 AM

Harbormaster completed remote builds in B185233: Diff 458185.Sep 6 2022, 9:47 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?

Address comments.

In D133327#3775081, @tschuett wrote:

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.

In D133327#3776434, @aemerson wrote:

In D133327#3775081, @tschuett wrote:

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).

Harbormaster completed remote builds in B185571: Diff 458666.Sep 8 2022, 1:05 AM

In D133327#3776446, @tschuett wrote:

In D133327#3776434, @aemerson wrote:

In D133327#3775081, @tschuett wrote:

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

13792ba4174f