Skip to content

[RISCV] Added definition of Ventana veyron-v1 processor. #65535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[RISCV] Added definition of Ventana veyron-v1 processor. #65535

wants to merge 2 commits into from

Conversation

mgudim
Copy link
Contributor

@mgudim mgudim commented Sep 6, 2023

No description provided.

@mgudim mgudim requested review from a team as code owners September 6, 2023 21:36
@github-actions github-actions bot added backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Sep 6, 2023
@mgudim mgudim requested review from topperc and asb September 6, 2023 21:37
@michaelmaitland michaelmaitland self-requested a review September 6, 2023 21:44
michaelmaitland
michaelmaitland reviewed Sep 6, 2023
View reviewed changes
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Sep 18, 2023
@michaelmaitland
Copy link
Contributor

Merge branch 'llvm:main' into mgudim_veyron_def

The LLVM GitHub User Guide recommends to rebase on main instead of merge main.

@mgudim
Copy link
Contributor Author

mgudim commented Sep 18, 2023 •
edited by michaelmaitland
Loading

Merge branch 'llvm:main' into mgudim_veyron_def

The LLVM GitHub User Guide recommends to rebase on main instead of merge main.

Yes, my bad. Still learning the new workflow. I got this merge commit after pressing the "sync" button on my fork. Is there a way to fix this now?

@michaelmaitland
Copy link
Contributor

Is there a way to fix this now?

I think you can drop the merge commit using git rebase -i. You may have to pass --rebase-merges to have the ability to drop the merge commit. Then you can pull upstream and git rebase upstream/main.

@mgudim
Copy link
Contributor Author

mgudim commented Sep 18, 2023

I think you can drop the merge commit using git rebase -i. You may have to pass --rebase-merges to have the ability to drop the merge commit. Then you can pull upstream and git rebase upstream/main.

Thanks Michael, I tried but messed it up (I see more changes than I intended). Let me try again. If I mess it up again, is it OK if I just close this PR and start over?

@michaelmaitland
Copy link
Contributor

michaelmaitland commented Sep 18, 2023 •
edited
Loading

I think you can drop the merge commit using git rebase -i. You may have to pass --rebase-merges to have the ability to drop the merge commit. Then you can pull upstream and git rebase upstream/main.

Thanks Michael, I tried but messed it up (I see more changes than I intended). Let me try again. If I mess it up again, is it OK if I just close this PR and start over?

I think you could drop 5fe9e82 using git rebase -i, recommit it with only the changes you want, and then follow the LLVM GitHub User Guide on how to rebase on main.

But if you want to close this PR and make another PR, thats fine by me.

@mgudim mgudim closed this Sep 18, 2023
@mgudim mgudim deleted the mgudim_veyron_def branch September 18, 2023 20:32
@dtcxzyw dtcxzyw mentioned this pull request Sep 19, 2023
Merged
mshockwave pushed a commit that referenced this pull request Aug 20, 2025
@dansalvato
[M68k] Fix reverse BTST condition causing opposite failure/success lo…
45e2c50 
…gic (#153086)

Given the test case:

```llvm
define fastcc i16 @testbtst(i16 %a) nounwind {
  entry:
    switch i16 %a, label %no [
      i16 11, label %yes
      i16 10, label %yes
      i16 9, label %yes
      i16 4, label %yes
      i16 3, label %yes
      i16 2, label %yes
    ]

  yes:
    ret i16 1

  no:
    ret i16 0
}
```

We currently get this result:

```asm
testbtst:                               ; @testbtst
; %bb.0:                                ; %entry
	move.l	%d0, %d1
	and.l	#65535, %d1
	sub.l	#11, %d1
	bhi	.LBB0_3
; %bb.1:                                ; %entry
	and.l	#65535, %d0
	move.l	#3612, %d1
	btst	%d0, %d1
	bne	.LBB0_3        ; <------- Erroneous condition
; %bb.2:                                ; %yes
	moveq	#1, %d0
	rts
.LBB0_3:                                ; %no
	moveq	#0, %d0
	rts
```

The cause of this is a line that explicitly reverses the `btst`
condition code. But on M68k, `btst` sets condition codes the same as
`and` with a bitmask, meaning `EQ` indicates failure (bit is zero) and
not success, so the condition does not need to be reversed.

In my testing, I've only been able to get switch statements to lower to
`btst`, so I wasn't able to explicitly test other options for lowering.
But (if possible to trigger) I believe they have the same logical error.
For example, in `LowerAndToBTST()`, a comment specifies that it's
lowering a case where the `and` result is compared against zero, which
means the corresponding `btst` condition should also not be reversed.

This patch simply flips the ternary expression in
`getBitTestCondition()` to match the ISD condition code with the same
M68k code, instead of the opposite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelmaitland michaelmaitland michaelmaitland left review comments

@topperc topperc Awaiting requested review from topperc

@asb asb Awaiting requested review from asb

Assignees
No one assigned
Labels
backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mgudim @michaelmaitland @llvmbot