This is an archive of the discontinued LLVM Phabricator instance.

[ARM] in LowerConstantFP, make sure we cover armv6-m execute-only
ClosedPublic

Authored by stuij on Jul 5 2023, 6:04 AM.

Details

Summary

Currently in LowerConstantFP we only check for the presence of an FPU after we
handle execute-only (XO), which would give us incorrect codegen for XO targets
that don't have an FPU. By moving the hasVFP3Base() check up, we guard against
this.

It doesn't look like we're currently getting here when generating code for v6m, but
we're guarding against the future.

Diff Detail

Event Timeline

stuij created this revision.Jul 5 2023, 6:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 6:04 AM
stuij requested review of this revision.Jul 5 2023, 6:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2023, 6:04 AM
stuij edited the summary of this revision. (Show Details)Jul 5 2023, 6:08 AM
stuij added reviewers: john.brawn, simonwallis2.
simonwallis2 accepted this revision.Jul 5 2023, 6:49 AM
This revision is now accepted and ready to land.Jul 5 2023, 6:49 AM
dmgreen added a subscriber: dmgreen.Jul 5 2023, 7:00 AM

Do you have a testcase that you can add?

stuij added a comment.Jul 5 2023, 7:14 AM

@dmgreen: see the description. It doesn't look like we get here currently when generating code.

I see. You are adding this for v6m, but v6m will never have legal FP types so will never get here?

We usually won't make changes like this if the code is already unreachable. Maybe turn them into a assert instead? In this case it looks fine though, presuming we don't want the combination of vfp2 and execute-only.

stuij updated this revision to Diff 538077.Jul 7 2023, 4:27 AM

address review comments

Thanks. Sounds good to me.

llvm/lib/Target/ARM/ARMISelLowering.cpp
7077

we -> We

7078–7079

Perhaps just use an assert instead of a llvm_unreachable? It's common to add the message to asserts in LLVM code.

stuij marked an inline comment as done.Jul 10 2023, 2:10 AM
stuij added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
7078–7079

According to the documentation, llvm_unreachable is preferable to an assert(0), which it effectively is in this case:
https://llvm.org/docs/CodingStandards.html#assert-liberally

There was a longish thread discussing this, and I still came away with unreachable being preferred.
https://discourse.llvm.org/t/llvm-unreachable-is-widely-misused/60587

dmgreen added inline comments.Jul 10 2023, 2:29 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
7078–7079

I meant:

assert((!ST->isThumb1Only() || ST->hasV8MBaselineOps()) && "Unexpected architecture");
stuij added inline comments.Jul 10 2023, 2:47 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
7078–7079

Yes, I get that, but reading the documentation:

"Use llvm_unreachable to mark a specific point in code that should never be reached. This is especially desirable for addressing warnings about unreachable branches, etc., but can be used whenever reaching a particular code path is unconditionally a bug (not originating from user input; see below) of some kind."

To me it reads that llvm_unreachable is preferable here.

dmgreen added inline comments.Jul 10 2023, 5:57 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
7078–7079

I don't think assert with a condition is discouraged, only assert(false) or assert(0). Those are better as llvm_unreachable

stuij updated this revision to Diff 538637.Jul 10 2023, 7:26 AM

addressed review comments

dmgreen accepted this revision.Jul 10 2023, 8:35 AM

Thanks. LGTM

This revision was landed with ongoing or failed builds.Jul 11 2023, 2:42 AM
This revision was automatically updated to reflect the committed changes.